Redis TTL #206

Talkabout · 2020-03-29T23:01:00Z

This is my first time changing unbound code so sorry for any "obvious" errors :)

Currently unbound does not provide any TTL information to Redis when storing values. This makes setups quite complex as every single redis instance needs to take care of eviction policies, which might not always be trivial to define. The code does 4 things:

makes sure that cache backend receives the ttl extracted from the dns respone in "store"-function
checks whether it makes sense to set a ttl on redis records. This is the case if either "serve-expired" is set to "no" or there is a value for "serve-expired-ttl".
in case "serve-expired-ttl" is set, this value is added to the ttl received to make sure, that there is even a chance to provide an expired record in the configured time frame
if ttl is to be set the redis command is extended with " EX " + ttl

I am running the code for a few days now and there are no issues so far. Which, of course, does not mean there there aren't any :)

some things to discuss:

I have used "sizeof(uint64_t)" to calculate the string length. This might be wrong.
I am using "EX" command extension for redis, which is available only with redis 2.6.12. Reason is that I wanted to have one command being executed and not multiple. "SETEX" seems to be kind of deprecated with the addition of "EX". Question here is, which redis versions need to be supported?

Thanks!

gthess

Nice work! Although I have left some remarks that need to be addressed in the code before merging.

Some features that I would also like to see with this PR (ordered by difficulty/amount of work):

Use of SETEX instead of SET .. EX .. . From the documentation SETEX is not deprecated and supported in earlier versions than SET EX.
Configuration option for redis to explicitly allow this behavior or not. (Default would be no, in order to maintain backwards compatibility)
Detect SETEX redis support when cachedb initiates and error out if not.

If you want to tackle them as well feel free! Otherwise let me know and I will add them with this PR.

cachedb/cachedb.c

cachedb/cachedb.h

cachedb/redis.c

Talkabout · 2020-03-30T15:12:59Z

HI @gthess ,

thanks for the review, I will tackle the requested changes on my end to get some experience with changing unbound code.

Bye

cachedb/redis.c

Talkabout · 2020-03-30T18:24:17Z

Hi @gthess ,

I have now added the changes. Creating a new option is quite a complex task... ;)

Now my git shows me the following files as changed:

cachedb/cachedb.c
cachedb/cachedb.h
cachedb/redis.c
util/config_file.c
util/config_file.h
util/configlexer.c
util/configlexer.lex
util/configparser.c
util/configparser.h
util/configparser.y

some of them are auto generated so the question is, if I should include all of them in my commit?

… redis records added check for redis command 'setex' when initializing redis connection updated documentation minor improvements to previous changes

Talkabout · 2020-03-31T10:59:09Z

I committed all changed files, please merge only the ones you think are necessary.

Thanks!

gthess · 2020-03-31T11:02:03Z

Thanks, from a first look everything seems good and in place!
But I do have some remarks. Will resolve my previous remarks and offer a new review later today.

Talkabout · 2020-03-31T11:03:52Z

Great! Thanks!

gthess

Nice almost done!
Apart from the remarks one editorial nit: I would use redis-expire-records instead of redis-set-ttl to avoid any confusion with the actual DNS TTL.

cachedb/redis.c

doc/unbound.conf.5.in

renamed option from 'redis-set-ttl' to 'redis-expire-records'

gthess · 2020-04-01T15:26:38Z

Thanks for sticking with this!

Talkabout · 2020-04-01T15:28:25Z

Thanks for your help and guidance!

* nlnet/master: (30 commits) - Merge PR NLnetLabs#214 from gearnode: unbound-control-setup recreate certificates. With the -r option the certificates are created again, without it, only the files that do not exist are created. fix unbound-control-setup is not idempotent - Keep track of number of timeouts. Use this counter to determine if capsforid fallback should be started. - More documentation for redis-expire-records option. - Changes for PR NLnetLabs#206 (formatting and remade lex and yacc output). changed init logic of redis backend as per review request implemented review feedback renamed option from 'redis-set-ttl' to 'redis-expire-records' added option 'redis-set-ttl' to define whether ttl should be added to redis records added check for redis command 'setex' when initializing redis connection updated documentation minor improvements to previous changes - Merge PR NLnetLabs#208: Fix uncached CLIENT_RESPONSE'es on stateful transports. Send tcp_req_info->spool_buffer as dnstap CLIENT_RESPONSE Fix uncached CLIENT_RESPONSE'es on stateful transports nroff fix for dash. - Merge PR NLnetLabs#207: Clarify if-automatic listens on 0.0.0.0 and :: Clarify if-automatic listens on 0.0.0.0 and :: honor 'server_expired_ttl' in redis added logic for redis to honor ttl when serve_expired is not enabled Changelog note for PR NLnetLabs#203. - Merge PR NLnetLabs#203 from noloader: Update README-Travis.md with current procedures. Make unbound-control error returned on missing domain name more user friendly. Update README-Travis.md with current procedures - Fix RPZ concurrency issue when using auth_zone_reload. ...

Talkabout added 3 commits March 29, 2020 15:22

added logic for redis to honor ttl when serve_expired is not enabled

1ec02f7

Merge branch 'master' of https://github.com/Talkabout/unbound

f66a519

honor 'server_expired_ttl' in redis

fa0623a

gthess requested changes Mar 30, 2020

View reviewed changes

gthess reviewed Mar 30, 2020

View reviewed changes

cachedb/redis.c Outdated Show resolved Hide resolved

added option 'redis-set-ttl' to define whether ttl should be added to…

b130a8b

… redis records added check for redis command 'setex' when initializing redis connection updated documentation minor improvements to previous changes

Talkabout requested a review from gthess March 31, 2020 11:00

gthess requested changes Mar 31, 2020

View reviewed changes

implemented review feedback

c25eb2c

renamed option from 'redis-set-ttl' to 'redis-expire-records'

Talkabout requested a review from gthess March 31, 2020 21:14

changed init logic of redis backend as per review request

e7b4529

gthess approved these changes Apr 1, 2020

View reviewed changes

gthess added a commit that referenced this pull request Apr 1, 2020

- Changes for PR #206 (formatting and remade lex and yacc output).

557a309

gthess merged commit 20aa782 into NLnetLabs:master Apr 1, 2020

gthess mentioned this pull request Apr 2, 2020

Option for Redis in cachedb to store records with expiry information #195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redis TTL #206

Redis TTL #206

Talkabout commented Mar 29, 2020

gthess left a comment

Talkabout commented Mar 30, 2020

Talkabout commented Mar 30, 2020 •

edited

Loading

Talkabout commented Mar 31, 2020

gthess commented Mar 31, 2020

Talkabout commented Mar 31, 2020

gthess left a comment

gthess commented Apr 1, 2020

Talkabout commented Apr 1, 2020

Redis TTL #206

Redis TTL #206

Conversation

Talkabout commented Mar 29, 2020

gthess left a comment

Choose a reason for hiding this comment

Talkabout commented Mar 30, 2020

Talkabout commented Mar 30, 2020 • edited Loading

Talkabout commented Mar 31, 2020

gthess commented Mar 31, 2020

Talkabout commented Mar 31, 2020

gthess left a comment

Choose a reason for hiding this comment

gthess commented Apr 1, 2020

Talkabout commented Apr 1, 2020

Talkabout commented Mar 30, 2020 •

edited

Loading