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

7900/melissavoegeli/dnsdist custom http response headers #7957

Conversation

@melissavoegeli
Copy link
Contributor

@melissavoegeli melissavoegeli commented Jun 19, 2019

Short description

Implements a solution for #7900 to allow custom response headers to be set with the addDOHLocal configuration. H2o provides a token table of acceptable headers. This PR utilizes that list as the allowable response headers. I still need to figure out how to do more appropriate warn logging and write a few more tests.

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)
@@ -1691,6 +1691,12 @@ void setupLuaConfig(bool client)
if (vars->count("serverTokens")) {
frontend->d_serverTokens = boost::get<const string>((*vars)["serverTokens"]);
}
if (vars->count("customResponseHeaders")) {
for (auto const& headerMap : boost::get<std::map<std::string,std::string>>((*vars)["customResponseHeaders"])) {
std::pair<std::string,std::string> headerResponse = std::make_pair(headerMap.first, headerMap.second);
Copy link
Contributor Author

@melissavoegeli melissavoegeli Jun 19, 2019

Is this a silly conversion? It feels silly, but I see pair getting used a lot.

Copy link
Member

@rgacogne rgacogne Jun 19, 2019

It's not, we want to be able to iterate quickly over the headers so a vector of pairs makes sense to me.

Copy link
Member

@rgacogne rgacogne Jun 20, 2019

I missed your question on IRC yesterday but we can iterate more quickly over a vector of pairs than a set or a map because the memory is contiguous. I'm not worried about duplicated entries, since you use a map in the configuration step, and even otherwise it would be up to the admin to be careful.

@rgacogne rgacogne added this to the dnsdist-1.4.0 milestone Jun 19, 2019
@rgacogne rgacogne self-requested a review Jun 19, 2019
Copy link
Member

@rgacogne rgacogne left a comment

Thanks a lot for this PR! If I'm not mistaken it would be better to use h2o_set_header_by_str() so we can set headers for which there is no token, but otherwise it looks very good.

@@ -1691,6 +1691,12 @@ void setupLuaConfig(bool client)
if (vars->count("serverTokens")) {
frontend->d_serverTokens = boost::get<const string>((*vars)["serverTokens"]);
}
if (vars->count("customResponseHeaders")) {
for (auto const& headerMap : boost::get<std::map<std::string,std::string>>((*vars)["customResponseHeaders"])) {
std::pair<std::string,std::string> headerResponse = std::make_pair(headerMap.first, headerMap.second);
Copy link
Member

@rgacogne rgacogne Jun 19, 2019

It's not, we want to be able to iterate quickly over the headers so a vector of pairs makes sense to me.

pdns/dnsdistdist/doh.cc Outdated Show resolved Hide resolved
pdns/dnsdistdist/doh.cc Outdated Show resolved Hide resolved
melissavoegeli and others added 5 commits Jun 19, 2019
No not allow silent conversion of NULL

Co-Authored-By: Remi Gacogne <rgacogne@users.noreply.github.com>
…tom_http_response_headers' into 7900/melissavoegeli/dnsdist_custom_http_response_headers
@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jul 1, 2019

Regression tests are currently failing with this error on Travis:

pycurl.error: (23, 'Failed writing header')

and the log shows:

TypeError: string argument expected, got 'bytes'

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jul 1, 2019

Regression tests are currently failing with this error on Travis:

On CircleCI, actually, sorry!

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jul 15, 2019

Hi! I'd love to merge this PR once the tests are fine, let us know if you need some help, or if you are just too busy at the moment :-)

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Aug 1, 2019

Superseded by #8148. Thanks a lot!

@rgacogne rgacogne closed this Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants