Use DNS record TTL and whether a record is in-use to determine expiration #7941
Conversation
brycekahle
added
changelog/no-changelog
component/system-probe
team/networks
[deprecated] qa/skip-qa - use other qa/ labels
| defer c.mux.Unlock() | ||
|
|
||
| for _, val := range c.data { | ||
| val.inUse = false |
There was a problem hiding this comment.
Would you please add a note perhaps here or at the type definition explaining the lifecycle of inUse. I don't quite follow how it works.
brycekahle
force-pushed
the
bryce.kahle/sysprobe-dns-ttl
branch
from
afb69a4
to
5e8c772
Compare
| continue | ||
| } | ||
|
|
||
| if bytes.Equal(domainQueried, record.Name) || | ||
| (alias != nil && bytes.Equal(alias, record.Name)) { | ||
| t.add(util.AddressFromNetIP(record.IP)) | ||
| t.add(util.AddressFromNetIP(record.IP), time.Duration(record.TTL)*time.Second) |
There was a problem hiding this comment.
Do we have an idea what the usual TTL might be for a DNS record? & if it's significantly longer than the current 3 min TTL, do we have an estimate for the impact this change might have on cache size?
There was a problem hiding this comment.
30s - 1hour is the usual range I've seen TTLs be in.
| @@ -232,9 +231,10 @@ func (s *SocketFilterSnooper) getCachedTranslation() *translation { | |||
|
|
|||
| // Recycle buffer if necessary | |||
| if t.ips == nil || len(t.ips) > maxIPBufferSize { | |||
| t.ips = make([]util.Address, 30) | |||
| t.ips = make(map[util.Address]time.Time, 30) | |||
There was a problem hiding this comment.
I don't understand this. Are we trying to resize the map and then copy over the entries?
There was a problem hiding this comment.
The translation object is allocated once and reused. This function is called to obtain the object for use, so we need to ensure it is "cleaned" up.
|
So DNS TTLs can be arbitrarily large. Should we set an upper limit on the ttl on the entries in the cache? |
@hmahmood We could, but we would be accepting that once it expired, the same issue this PR is intended to fix, would happen. We have no indication what the DNS client is using as a max TTL or when it purges an entry from its cache. We do also have a maximum size on this DNS cache. |
brycekahle
force-pushed
the
bryce.kahle/sysprobe-dns-ttl
branch
from
596d563
to
e348948
Compare
What does this PR do?
The reverse DNS cache inside system-probe was previously caching for 3 minutes, and extending that duration every time a record was used. This uses the actual TTL from the record and keeps track of values in-use.
Motivation
More accurate DNS caching behavior.
Additional Notes
This does not handle the following case:
myhostto1.1.1.1with a TTL of 30s1.1.1.1and kept open1.1.1.1The connection from step 4 will use the same domain mapping from step 1, even though the record has expired, and the new connection did not resolve any DNS.
To fix this scenario would require more sophisticated tracking of connections to DNS records.
Multiple clients also disrupt the record expiration by potentially causing an early expiration before the other client has had a chance to use the value. Fixing this would require per-client tracking with some sort of inactivity limit to prevent a single client request (often from debugging) from keeping records around forever.