Draft PR for #6722 -- cache TTLs for individual IP addresses in DNS responses. #6732
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,11 +76,16 @@ func (fs *fqdnSelectorItem) matches(fqdn string) bool { | |
| // expirationTime of the records, which is the DNS response | ||
| // receiving time plus lowest applicable TTL. | ||
| type dnsMeta struct { | ||
| //expirationTime time.Time | ||
| // Key for responseIPs is the string representation of the IP. | ||
| // It helps to quickly identify IP address updates when a | ||
| // new DNS response is received. | ||
| responseIPs map[string]ipWithTTL | ||
antoninbas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| type ipWithTTL struct { | ||
| ip net.IP | ||
| expirationTime time.Time | ||
| } | ||
|
|
||
| // subscriber is a entity that subsribes for datapath rule realization | ||
|
|
@@ -253,8 +258,8 @@ func (f *fqdnController) getIPsForFQDNSelectors(fqdns []string) []net.IP { | |
| } | ||
| for fqdn := range fqdnsMatched { | ||
| if dnsMeta, ok := f.dnsEntryCache[fqdn]; ok { | ||
| for _, ipWithMetaData := range dnsMeta.responseIPs { | ||
|
There was a problem hiding this comment. nit: s/ipWithMetaData/ipData |
||
| matchedIPs = append(matchedIPs, ipWithMetaData.ip) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -405,80 +410,118 @@ func (f *fqdnController) deleteRuleSelectedPods(ruleID string) error { | |
|
|
||
| func (f *fqdnController) onDNSResponse( | ||
| fqdn string, | ||
| responseIPs map[string]ipWithTTL, | ||
| waitCh chan error, | ||
| ) { | ||
| currentTime := time.Now() | ||
| if len(responseIPs) == 0 { | ||
| klog.V(4).InfoS("FQDN was not resolved to any addresses, skip updating DNS cache", "fqdn", fqdn) | ||
| if waitCh != nil { | ||
| waitCh <- nil | ||
| } | ||
| return | ||
| } | ||
|
|
||
| addressUpdate := false | ||
| f.fqdnSelectorMutex.Lock() | ||
| defer f.fqdnSelectorMutex.Unlock() | ||
| oldDNSMeta, exist := f.dnsEntryCache[fqdn] | ||
| ipMetaDataHolder := make(map[string]ipWithTTL) | ||
|
There was a problem hiding this comment. nit: move the variable definitions before the lock, and keep There was a problem hiding this comment. @Dyanngg Thank you , that was helpful in sense that i did realise that the code looked more readable with ease after this. |
||
| minTimeToReQuery := time.Unix(1<<63-62135596801, 999999999) | ||
|
|
||
| minTime := func(t1, t2 time.Time) time.Time { | ||
|
There was a problem hiding this comment. nit: func earlierOf(t1, t2) / laterOf(t1, t2) There was a problem hiding this comment. @Dyanngg Yes, the names make more meaning. |
||
| if t1.Before(t2) { | ||
| return t1 | ||
| } | ||
| return t2 | ||
| } | ||
|
|
||
| maxTime := func(t1, t2 time.Time) time.Time { | ||
| if t1.After(t2) { | ||
| return t1 | ||
| } | ||
| return t2 | ||
| } | ||
|
|
||
| if exist { | ||
| // Data related to this domain exists in the cache. | ||
| // Besides presence of existing IPs, two scenarios : 1) May get New IPs 2) Some old IPs may be absent. | ||
|
|
||
| // check for new IPs and these new IPs need to be added with its new TTL value as received in response. | ||
| for ipStr, ipMeta := range responseIPs { | ||
| if _, exist := oldDNSMeta.responseIPs[ipStr]; !exist { | ||
| ipMetaDataHolder[ipStr] = ipWithTTL{ | ||
| ip: ipMeta.ip, | ||
| expirationTime: ipMeta.expirationTime, | ||
| } | ||
| minTimeToReQuery = minTime(ipMeta.expirationTime, minTimeToReQuery) | ||
| } | ||
| } | ||
|
|
||
| for ipStr, ipMeta := range oldDNSMeta.responseIPs { | ||
| if _, exist := responseIPs[ipStr]; !exist { | ||
| if ipMeta.expirationTime.Before(currentTime) { | ||
| // this ip is expired and stale, remove it by not including it. | ||
| addressUpdate = true | ||
| } else { | ||
| // It hasn't expired yet, so just retain it with its existing expirationTime. | ||
| ipMetaDataHolder[ipStr] = ipMeta | ||
| minTimeToReQuery = minTime(ipMeta.expirationTime, minTimeToReQuery) | ||
| } | ||
| } else { | ||
| // This old IP also exists in current response, so update it with max time between received time and its old cached time. | ||
| expTime := maxTime(responseIPs[ipStr].expirationTime, oldDNSMeta.responseIPs[ipStr].expirationTime) | ||
| ipMetaDataHolder[ipStr] = ipWithTTL{ | ||
| ip: ipMeta.ip, | ||
| expirationTime: expTime, | ||
| } | ||
| minTimeToReQuery = minTime(expTime, minTimeToReQuery) | ||
| } | ||
| } | ||
|
|
||
| } else { | ||
| // First time seeing this domain. | ||
| // check if this needs to be tracked, by checking its presence in the rules. | ||
| // If a FQDN policy had been applied then there must be rule records but because it's | ||
| // not in cache hence its FQDN:SelectorItem mapping may not be present. | ||
| for selectorItem := range f.selectorItemToRuleIDs { | ||
| // Only track the FQDN if there is at least one fqdnSelectorItem matching it. | ||
| if selectorItem.matches(fqdn) { | ||
| f.setFQDNMatchSelector(fqdn, selectorItem) | ||
| for ipStr, ipMeta := range responseIPs { | ||
| ipMetaDataHolder[ipStr] = ipWithTTL{ | ||
| ip: ipMeta.ip, | ||
| expirationTime: ipMeta.expirationTime, | ||
| } | ||
| minTimeToReQuery = minTime(ipMeta.expirationTime, minTimeToReQuery) | ||
|
There was a problem hiding this comment. Since thees lines appear four times at least, might make sense to make it a helper. There was a problem hiding this comment. @Dyanngg Yes, that was a nice improvement. |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(ipMetaDataHolder) != 0 { | ||
| addressUpdate = true | ||
|
There was a problem hiding this comment. I don't think this is correct. Isn't There was a problem hiding this comment. when no IP is changed, I think the map is not empty, why this is set to true? |
||
| f.dnsEntryCache[fqdn] = dnsMeta{ | ||
| responseIPs: ipMetaDataHolder, | ||
| } | ||
| // The FQDN will be added to the queue only after `lowestTTL` value which | ||
| // would already have been derived using the minTTL logic. | ||
| f.dnsQueryQueue.AddAfter(fqdn, minTimeToReQuery.Sub(currentTime)) | ||
| } | ||
| f.syncDirtyRules(fqdn, waitCh, addressUpdate) | ||
| } | ||
|
|
||
| // onDNSResponseMsg handles a DNS response message intercepted. | ||
| func (f *fqdnController) onDNSResponseMsg(dnsMsg *dns.Msg, waitCh chan error) { | ||
| fqdn, responseIPs, err := f.parseDNSResponse(dnsMsg) | ||
| if err != nil { | ||
| klog.V(2).InfoS("Failed to parse DNS response") | ||
| if waitCh != nil { | ||
| waitCh <- fmt.Errorf("failed to parse DNS response: %v", err) | ||
| } | ||
| return | ||
| } | ||
| f.onDNSResponse(fqdn, responseIPs, waitCh) | ||
| } | ||
|
|
||
| // syncDirtyRules triggers rule syncs for rules that are affected by the FQDN of DNS response | ||
|
|
@@ -594,38 +637,54 @@ func (f *fqdnController) runRuleSyncTracker(stopCh <-chan struct{}) { | |
| } | ||
|
|
||
| // parseDNSResponse returns the FQDN, IP query result and lowest applicable TTL of a DNS response. | ||
| func (f *fqdnController) parseDNSResponse(msg *dns.Msg) (string, map[string]ipWithTTL, error) { | ||
| if len(msg.Question) == 0 { | ||
| return "", nil, fmt.Errorf("invalid DNS message") | ||
| } | ||
| fqdn := strings.ToLower(msg.Question[0].Name) | ||
| // Case 1: In the upcoming patch an admin would set this to a maximum value, which will be read from the configuration. | ||
|
There was a problem hiding this comment. note that in with the minTTL feature, expirationTime will be There was a problem hiding this comment. @Dyanngg Get it now, and indeed i was actually getting a little confused as i was trying to put the upcoming patch related modifications here, which i understand now, will happen in upcoming days when i start working on the patch itself. |
||
| maxConfiguredTTL := uint32(math.MaxUint32) // a TTL must exist in the RRs | ||
hkiiita marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| responseIPs := map[string]ipWithTTL{} | ||
| currentTime := time.Now() | ||
| for _, ans := range msg.Answer { | ||
| switch r := ans.(type) { | ||
| case *dns.A: | ||
| if f.ipv4Enabled { | ||
| // So, using case 1 above , we may not make below comparison and just assign the max value as expirationTime ; ignoring the incoming dns ttl. | ||
| var expirationTime uint32 | ||
| if r.Header().Ttl < maxConfiguredTTL { | ||
Dyanngg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| expirationTime = maxConfiguredTTL | ||
| } else { | ||
| expirationTime = r.Header().Ttl | ||
| } | ||
| responseIPs[r.A.String()] = ipWithTTL{ | ||
| ip: r.A, | ||
| expirationTime: currentTime.Add(time.Duration(expirationTime) * time.Second), | ||
| } | ||
|
|
||
| } | ||
| case *dns.AAAA: | ||
| if f.ipv6Enabled { | ||
| var expirationTime uint32 | ||
| if r.Header().Ttl < maxConfiguredTTL { | ||
| expirationTime = maxConfiguredTTL | ||
| } else { | ||
| expirationTime = r.Header().Ttl | ||
| } | ||
| responseIPs[r.AAAA.String()] = ipWithTTL{ | ||
| ip: r.AAAA, | ||
| expirationTime: currentTime.Add(time.Duration(expirationTime) * time.Second), | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if len(responseIPs) > 0 { | ||
| klog.V(4).InfoS("Received DNS Packet with valid Answer", "IPs", responseIPs, "TTL", maxConfiguredTTL) | ||
| } | ||
| if strings.HasSuffix(fqdn, ".") { | ||
| fqdn = fqdn[:len(fqdn)-1] | ||
| } | ||
| return fqdn, responseIPs, nil | ||
| } | ||
|
|
||
| func (f *fqdnController) worker() { | ||
|
|
@@ -662,24 +721,27 @@ func (f *fqdnController) lookupIP(ctx context.Context, fqdn string) error { | |
|
|
||
| var errs []error | ||
|
|
||
| makeResponseIPs := func(ips []net.IP) map[string]ipWithTTL { | ||
| responseIPs := make(map[string]ipWithTTL) | ||
| for _, ip := range ips { | ||
| responseIPs[ip.String()] = ipWithTTL{ | ||
| ip: ip, | ||
| expirationTime: time.Now().Add(time.Duration(defaultTTL) * time.Second), | ||
| } | ||
| } | ||
| return responseIPs | ||
| } | ||
|
|
||
| if f.ipv4Enabled { | ||
| if ips, err := resolver.LookupIP(ctx, "ip4", fqdn); err == nil { | ||
| f.onDNSResponse(fqdn, makeResponseIPs(ips), nil) | ||
| } else { | ||
| errs = append(errs, fmt.Errorf("DNS request failed for IPv4: %w", err)) | ||
| } | ||
| } | ||
| if f.ipv6Enabled { | ||
| if ips, err := resolver.LookupIP(ctx, "ip6", fqdn); err == nil { | ||
| f.onDNSResponse(fqdn, makeResponseIPs(ips), nil) | ||
| } else { | ||
| errs = append(errs, fmt.Errorf("DNS request failed for IPv6: %w", err)) | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There was a problem hiding this comment. these changes are unrelated to the rest of the PR, and should be removed |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove if no longer needed