Skip to content
This repository has been archived by the owner on Mar 2, 2024. It is now read-only.

fix(hostedzone): Set NS record's TTL to 900 #231

Merged
merged 5 commits into from
Jan 7, 2021
Merged

Conversation

yngvark
Copy link
Contributor

@yngvark yngvark commented Jan 6, 2021

Description

Set TTL of NS record to 15 minutes when creating a hosted zone.

Motivation and Context

When users delete a cluster, and creates it again with the same domain name, okctl create fails. This is due to DNS resolvers having cached a now outdated NS record for the domain. The step immediately after creating a hosted zone, create cognito identity pool, fails.

And because users frequently creates and deletes clusters when testing okctl for the first x times, they will run into this quite often.

The current problem before this PR is something like this (not 100% sure here): After a delete cluster, and when trying to create it again, oslo.systems's NS records correctly points to the new NS record of mycluster.oslo.systems. However, the NS record of the old mycluster.oslo.systems had TTL two days, meaning DNS resolvers still will return the old NS record. So when okctl create (or anyone) attempts to use mycluster.oslo.systems for anything, DNS resolvers will ask the old NS name servers to do lookups, making any request fail to mynewcluster.oslo.systems fail. For instance, getting the A-record of myapp.mycluster.oslo.systems will resolve to the IP of myapp in my previous cluster.

By setting the TTL to 900 seconds, the user can just wait 15 minutes, instead of two days, after deleting a cluster before creating it again.

Why 900 secs / 15 minutes? Because this observation: https://blog.apnic.net/2019/11/12/stop-using-ridiculously-low-dns-ttls/

The number of required queries drops from 47% to 34% by setting a minimum TTL of 5 minutes, to 25% with a 15-minute minimum, and to 13% with a 1-hour minimum.

Our users can increase to 1 or 2 hours later when they are done testing, 15 minutes seems like a good sweet spot to facilitate multiple runs of okctl and a inefficient and too low TTL.

How Has This Been Tested?

Manually.

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the release notes (for the next release).

Copy link
Member

@deifyed deifyed 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!

Maybe we could add a default value to the NSTTL somewhere? Not sure if its needed, I'd just prefer to not have to read deep into DNS RFC's to figure out a sane value when using the create hosted zone function in the future

@yngvark yngvark marked this pull request as ready for review January 7, 2021 10:45
@yngvark yngvark requested a review from ivaruf January 7, 2021 10:45
Copy link
Contributor

@ivaruf ivaruf 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 👍

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #231 (296e291) into master (e13a4ca) will decrease coverage by 0.24%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
- Coverage   38.51%   38.26%   -0.25%     
==========================================
  Files         102      102              
  Lines        2942     2961      +19     
==========================================
  Hits         1133     1133              
- Misses       1809     1828      +19     
Impacted Files Coverage Δ
pkg/api/core/service_domain.go 0.00% <0.00%> (ø)
pkg/route53/route53.go 20.00% <0.00%> (-18.10%) ⬇️
pkg/api/core/service_certificate_api.go
pkg/api/core/service_cluster_api.go
pkg/api/core/service_vpc_api.go
pkg/api/core/service_domain_api.go
pkg/api/core/service_parameter_api.go
pkg/api/core/service_identitymanager_api.go
pkg/apis/okctl.io/v1alpha1/cluster_v1alpha1.go
pkg/client/store/filesystem_store.go
... and 10 more

@sonarcloud
Copy link

sonarcloud bot commented Jan 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yngvark yngvark merged commit a8aaf65 into master Jan 7, 2021
@yngvark yngvark deleted the KM117-set_ns_ttl_to_900 branch January 7, 2021 12:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants