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

Redis TTL #206

Merged
merged 6 commits into from Apr 1, 2020
Merged

Redis TTL #206

merged 6 commits into from Apr 1, 2020

Conversation

Talkabout
Copy link
Contributor

@Talkabout Talkabout commented Mar 29, 2020

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:

  1. makes sure that cache backend receives the ttl extracted from the dns respone in "store"-function
  2. 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".
  3. 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
  4. 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:

  1. I have used "sizeof(uint64_t)" to calculate the string length. This might be wrong.
  2. 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!

Copy link
Member

@gthess gthess left a comment

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 Outdated Show resolved Hide resolved
cachedb/cachedb.c Outdated Show resolved Hide resolved
cachedb/cachedb.h Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
@Talkabout
Copy link
Contributor Author

Talkabout commented Mar 30, 2020

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 Outdated Show resolved Hide resolved
@Talkabout
Copy link
Contributor Author

Talkabout commented Mar 30, 2020

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
Copy link
Contributor Author

Talkabout commented Mar 31, 2020

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

Thanks!

@Talkabout Talkabout requested a review from gthess Mar 31, 2020
@gthess
Copy link
Member

gthess commented Mar 31, 2020

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
Copy link
Contributor Author

Talkabout commented Mar 31, 2020

Great! Thanks!

Copy link
Member

@gthess gthess left a comment

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 Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
cachedb/redis.c Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
renamed option from 'redis-set-ttl' to 'redis-expire-records'
@Talkabout Talkabout requested a review from gthess Mar 31, 2020
gthess
gthess approved these changes Apr 1, 2020
@gthess gthess merged commit 20aa782 into NLnetLabs:master Apr 1, 2020
@gthess
Copy link
Member

gthess commented Apr 1, 2020

Thanks for sticking with this!

@Talkabout
Copy link
Contributor Author

Talkabout commented Apr 1, 2020

Thanks for your help and guidance!

jedisct1 added a commit to jedisct1/unbound that referenced this pull request Apr 13, 2020
* 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.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants