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

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

Merged
merged 8 commits into from Jan 26, 2021

Conversation

rijswijk
Copy link
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>).

Copy link
Contributor

@ralphdolmans ralphdolmans 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! I have added some comments.

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

util/data/msgreply.c Show resolved Hide resolved
util/data/msgencode.c Show resolved Hide resolved
}
if(d->ttl < now)
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 */
Copy link
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
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.

Copy link
Member

@wcawijngaards wcawijngaards 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, with comment on comparison in packed_rrset.c improvement.

Copy link
Member

@gthess gthess left a comment

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
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.

None yet

4 participants