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

Sensible TTL for DNS names #4106

Merged
merged 3 commits into from
Oct 27, 2022
Merged

Sensible TTL for DNS names #4106

merged 3 commits into from
Oct 27, 2022

Conversation

coot
Copy link
Contributor

@coot coot commented Oct 26, 2022

  • Fixed a few typos and code layout
  • DNSActions: refactor code
  • dns-actions: make sure TTLs are strictly greater than 0

Description

description of the pull request, if it fixes a particular issue it should link
the PR to a particular issue, see
ref

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

Copy link
Contributor

@njd42 njd42 left a comment

Choose a reason for hiding this comment

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

LGTM - mapping small TTLs to 30 would seem sensible.

--


fixupTTL :: DNS.TTL -> DNS.TTL
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that we map all other positive numbers below, say, 30 to 30.

There is no reason for us to have a TTL less than that - we don't want to ever churn faster than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my original suggestion :)

Otherwise the local root monitoring thread will busy loop.
@coot coot marked this pull request as ready for review October 27, 2022 09:13
@coot
Copy link
Contributor Author

coot commented Oct 27, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 27, 2022

@iohk-bors iohk-bors bot merged commit e802890 into master Oct 27, 2022
@iohk-bors iohk-bors bot deleted the coot/dns-ttl branch October 27, 2022 12:52
iohk-bors bot added a commit that referenced this pull request Nov 9, 2022
4120: Cherry picked network changes for cardano-node-1.35.5 release r=coot a=coot

This cherry-picked patches from the following PRs:

* #3794
* #3844
* #3785
* #3904
* #3915
* #3852
* #3970
* #3979
* #4015
* #4067
* #4004
* #4086
* #4113
* #4106
* #4127
* #4103

Also cherry-picked almost all the commits which modify GitHub actions:
* 18c5244 Run GitHub Actions on pull requests   
* 3adf5a9 Use newer version of io-sim           
* ee9b7a6 Fix GH Actions Windows CI: switch from pkgconf to pkg-config 
* e6cf074 github-actions: use `ubuntu-latest`   
* 9a8b959 Updated versions of github actions    
* fc8f8f0 Fix GH Actions Windows CI caching     
* 7f07c40 Windows Github Actions now use MSYS2  
* b21a7ce Fix chocolatey CI error
* #4134               

TODO:

* [x] bump versions of packages
* [x] input-output-hk/cardano-haskell-packages#84

Co-authored-by: Mark Tullsen <[email protected]>
Co-authored-by: Marcin Szamotulski <[email protected]>
@coot coot added the DNS Issues / PRs related to DNS label Jul 6, 2023
@coot coot changed the title coot/dns ttl Sensible TTL for DNS names Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNS Issues / PRs related to DNS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants