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

dnsdist: Add a new chain of rules triggered after cache insertion #12280

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Dec 6, 2022

Short description

The general idea is to be able to store the unedited version into the cache while delivering a different version to the actual client. This is useful when one is sending different answers to different clients, like when dealing with abuse traffic, but still want to be able to cache the initial response from the backend. We already have a chain of rules that are triggered after a cache-hit, but until now we lacked the ability to trigger after getting the response corresponding to a cache-miss.

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)

The general idea is to be able to store the unedited version into
the cache while delivering a different version to the actual client.
This is useful when one is sending different answers to different
clients, like when dealing with abuse traffic, but still want to be
able to cache the initial response from the backend.
We already have a chain of rules that are triggered after a cache-hit,
but until now we lacked the ability to trigger after getting the
response corresponding to a cache-miss.
@rgacogne
Copy link
Member Author

rgacogne commented Dec 8, 2022

CI failures are unrelated.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

looks good, one question just to be sure

@@ -333,18 +349,30 @@ void setupLuaRules(LuaContext& luaCtx)
return rulesToString(getTopRules(*rules, top.get_value_or(10)), vars);
});

luaCtx.writeFunction("getCacheHitResponseRules", [](boost::optional<unsigned int> top) {
Copy link
Member

Choose a reason for hiding this comment

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

was the intention to fully remove this function (and topCacheHitRules)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I intended to rename them, yes. The naming was inconsistent with the existing functions dealing with the other chains of rules, and the documentation was using these names already. I pondered deprecating these but as are mostly used to inspect rules at run-time, I don't think it will break any existing setup.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! I see the 'new' names in the old docs now. Great :)

@rgacogne rgacogne merged commit 6569520 into PowerDNS:master Dec 9, 2022
@rgacogne rgacogne deleted the ddist-cache-inserted-rules branch December 9, 2022 14:43

Return the cache-inserted response rules that matched the most.

:param int top: How many response rules to return.
Copy link
Member

Choose a reason for hiding this comment

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

Default value of top missing


This function shows the cache-inserted response rules that matched the most.

:param int top: How many rules to show.
Copy link
Member

Choose a reason for hiding this comment

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

Default value of top missing

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

Successfully merging this pull request may close these issues.

3 participants