Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add feature to return the original instead of a decrementing TTL - #275

#275
Merged
gthess merged 8 commits into
NLnetLabs:masterNLnetLabs/unbound:masterfrom
rijswijk:orig_ttlCopy head branch name to clipboard
Jan 26, 2021
Merged

Add feature to return the original instead of a decrementing TTL#275
gthess merged 8 commits into
NLnetLabs:masterNLnetLabs/unbound:masterfrom
rijswijk:orig_ttlCopy head branch name to clipboard

Conversation

@rijswijk

Copy link
Copy Markdown
Contributor

This PR adds a feature called serve-original-ttl which, if enabled, will make Unbound return the original TTL for records as received from upstream. This feature may be useful if Unbound serves as a front-end to, e.g., a hidden authoritative name server.

Notes on the PR:

  • The caching logic was not changed, the TTL values in packed_rrset_data structures are treated the same as before. Instead, the new code stores an additional value in this structure call ttl_add which records how much has been added to the TTLs in the structure, and can be subtracted from these absolute TTLs instead of the current time when encoding responses.
  • The logic in packed_rrset_encode in msgencode.c was changed slightly to make the time adjustment explicit. The original code sometimes modified the value of the function's input parameter timenow, setting it to 0. With the introduction of the new feature, this would have led to messy conditionals when adjusting the TTLs during encoding.
  • Based on feedback from @wcawijngaards on an earlier version of this code the logic when decrementing TTLs in cachedb/cachedb.c was also changed such that the semantics of the decrement should be preserved while the original TTL will still be returned if this feature is enabled.
  • The manual page explicitly specifies that enabling this feature means Unbound will ignore cache-min-ttl and cache-max-ttl.
  • When allocating space for struct packed_rrset_data the code was changed in several places to explicitly initialise the newly allocated memory to zero, using either regional_alloc_zero or calloc(1, <size>).

@rijswijk
rijswijk requested review from gthess and ralphdolmans July 30, 2020 09:48

@ralphdolmans ralphdolmans left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! I have added some comments.

It would be nice to also have a testbound test that makes use of this option.

Comment thread util/data/msgreply.c
Comment thread util/data/msgencode.c
Comment thread util/data/packed_rrset.c
d->ttl = SERVE_EXPIRED?SERVE_EXPIRED_REPLY_TTL:0;
else d->ttl -= now;
else d->ttl -= SERVE_ORIGINAL_TTL ? data->ttl_add : now;
d->ttl_add = 0; /* TTLs have been made relative */

@ralphdolmans ralphdolmans Jul 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For my understanding, is the d->ttl_add = 0 here "just to be safe", or really needed? Guess it is fine to have it either way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since I don't know what will happen to the data in struct packed_rrset_data I added this just to be safe.

@wcawijngaards wcawijngaards left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, with comment on comparison in packed_rrset.c improvement.

@gthess gthess left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is also a go from me!
I'll add a testcase for this and merge it. Is this OK @rijswijk?

@rijswijk

Copy link
Copy Markdown
Contributor Author

Yup @gthess go ahead, thanks!

gthess added a commit that referenced this pull request Jan 26, 2021
- Merge PR #275 by Roland van Rijswijk-Deij, Add feature to return the
  original instead of a decrementing TTL ('serve-original-ttl').
@gthess
gthess merged commit f5b7169 into NLnetLabs:master Jan 26, 2021
jedisct1 added a commit to jedisct1/unbound that referenced this pull request Jan 30, 2021
* nlnet/master: (34 commits)
  - Fix for doxygen 1.8.20 compatibility.
  - Fix fwd ancil test post script when not supported.
  - Fix empty clause warning in edns pass for padding.
  - Fix to use correct type for label count in ipdnametoaddr rpz routine.
  - Fix empty clause warning in config_file nsid parse.
  - Fix to use correct type for label count in rpz routine.
  - Annotate that we ignore the return value of if_indextoname.
  - Fix compile of unbound-dnstap-socket without dnstap installed.
  - Ignore cache blacklisting when trying to reply with expired data from   cache. (NLnetLabs#394)
  Changelog entry for: - Merge PR NLnetLabs#355 from noloader: Make ICANN Update CA and DS Trust Anchor   static data.
  Changelog entry for: - Merge PR NLnetLabs#275 by Roland van Rijswijk-Deij, Add feature to return the   original instead of a decrementing TTL ('serve-original-ttl').
  Changelog entry for: - Merge PR NLnetLabs#408 from fobser: Prevent a few more yacc clashes.
  - Update example.con.in and add a testcase for PR NLnetLabs#275.
  Some review nits from George
  Test some different padding sizes
  padding.tdir text in single TXT RR
  tdir test for padding option
  Addressed review comment from @wcawijngaards
  Changelog entry for padding option
  Move NSID Changelog entry to day of merge
  ...
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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.