Cache TTLs for individual IP addresses in DNS responses. #6732

hkiiita · 2024-10-11T06:02:10Z

This is a draft PR for work towards #6722 .

pkg/agent/controller/networkpolicy/fqdn.go

test/e2e/fqdn_dns_cache_test.go

pkg/agent/controller/networkpolicy/fqdn.go

Dyanngg · 2024-10-17T18:55:42Z

pkg/agent/controller/networkpolicy/fqdn.go

+					ipMetaDataHolder[ipStr] = ipWithTTL{
+						ip:             ipMeta.ip,
+						expirationTime: ipMeta.expirationTime,
+					}
+					minTimeToReQuery = minTime(ipMeta.expirationTime, minTimeToReQuery)


Since thees lines appear four times at least, might make sense to make it a helper.

@Dyanngg Yes, that was a nice improvement.

antoninbas · 2024-10-18T16:39:59Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -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
+	//expirationTime time.Time


remove if no longer needed

pkg/agent/controller/networkpolicy/fqdn.go

antoninbas · 2024-10-18T16:43:54Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -253,8 +258,8 @@ func (f *fqdnController) getIPsForFQDNSelectors(fqdns []string) []net.IP {
 		}
 		for fqdn := range fqdnsMatched {
 			if dnsMeta, ok := f.dnsEntryCache[fqdn]; ok {
-				for _, ip := range dnsMeta.responseIPs {
-					matchedIPs = append(matchedIPs, ip)
+				for _, ipWithMetaData := range dnsMeta.responseIPs {


nit: s/ipWithMetaData/ipData

antoninbas · 2024-10-18T16:47:29Z

pkg/agent/controller/networkpolicy/fqdn.go

+
+	currentTime := time.Now()
+	ipWithTTLMap := make(map[string]ipWithTTL)
+	minTimeToReQuery := time.Unix(1<<63-62135596801, 999999999)


what time is this supposed to be? At the very least there should be a comment to explain. But based on usage, I am not sure why we are not just using the zero value for time.Time:

var minTimeToReQuery time.Time

@antoninbas Actually ,

minTimeToReQuery := time.Unix(1<<63-62135596801, 999999999)

i had chosen above expression to establish a reference point for tracking the minimum time to re-query DNS requests as IPs expire. Since our goal in this PR for this bug/issue is to send requests as and when an IP expires, initializing minTimeToReQuery to a maximum time value allows us to compare and update it as we process various time values , ultimately allowing us to determine the earliest re-query time.

What you could do though is to make earlierOf take time.Time pointers as parameters, and return the non-nil value if one of t1/t2 is nil. That way you would not need to initialize minTimeToReQuery. Note that in that case the return value should also be a pointer IMO (since earlierOf(nil, nil) is nil), and when it's used eventually you should check that minTimeToReQuery is not nil

Thanks @Dyanngg , yes i think using that approach we can avoid initialising minTimeToReQuery .

I am fine with the pointer solution, but it seems to me that we could just pick a reasonable time such as now + 24*time.Hour and use that (with an appropriate comment of course). The time is only used if len(ipWithTTLMap) > 0 anyway. And it seems reasonable to resend a query after 24h even if we don't really need to? At least it won't impact correctness.

Yes, i think this was the point i was missing . Maybe i had set it to too large a value under assumption that i needed a very large value. The 24 hour time frame seems more reasonable as a replacement to existing large value that i had put.

antoninbas · 2024-10-18T16:51:18Z

pkg/agent/controller/networkpolicy/fqdn.go

+	minTimeToReQuery := time.Unix(1<<63-62135596801, 999999999)
+	addressUpdate := false
+
+	ipMapFiller := func(ip string, ipMeta ipWithTTL, minTime *time.Time) {


the name of this function (ipMapFiller) is really hurting the readability of the code. I would suggest something like addIPtoMap / addIPToCache / addIPToNewCache / setIPInMap / ...

Additionally, minTime does not need to be a parameter to this function: you are always using &minTimeToReQuery as the argument value, and minTimeToReQuery is captured by the closure (by reference).

Yes , my bad. The closure captures the variable minTimeToReQuery and should not be passed as an argument.

antoninbas · 2024-10-18T16:52:07Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+					t.Errorf("Expected TTL for %s to be %v, got %v", ipStr, expectedTTL, ipMeta.expirationTime)
+				}
+			}
+			println("\n\n")


don't use a print statement in a test like this

antoninbas · 2024-10-18T16:54:36Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+						// sample IP with time simulating expired time, i thought that using negative time will
+						// simulate expired time as it will always equate  to before when compared to any time.


be more concise in your comments:

// expired IP that should be removed from the cache when onDNSResponse is called

antoninbas · 2024-10-18T16:56:14Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+				// we get new IP
+				"192.0.2.3": {ip: net.ParseIP("192.0.2.3"), expirationTime: currentTime.Add(10 * time.Second)},
+				// and an exisiting IP
+				"192.0.2.1": {ip: net.ParseIP("192.0.2.1"), expirationTime: currentTime.Add(10 * time.Second)},


isn't the test more useful if the new TTL for this IP is different from the current one? If the new TTL in the response is greater than the existing one, the new one should be sued. If it is less than the existing one, the current one should remain in use.

antoninbas · 2024-10-18T16:58:24Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			},
+		},
+		{
+			name: "old IP expired",


this is a strict subset of the previous test, and doesn't need to be included

there are 2 possible approaches here:

have a single unit test case with multiple different IPs, ensuring that all the branches / cases of the onDNSResponse are tested

have multiple test cases, each testing a different branch / scenario

Usually, we prefer the second approach because it makes test failures easier to identify / troubleshoot.

antoninbas · 2024-10-18T16:59:34Z

test/e2e/framework.go

these changes are unrelated to the rest of the PR, and should be removed

Dyanngg · 2024-10-22T02:26:18Z

pkg/agent/controller/networkpolicy/fqdn.go

 				f.setFQDNMatchSelector(fqdn, selectorItem)
+				for ipStr, ipMeta := range newDNSresponseIPs {


This does not make sense to be in part of the for loop... We don't want to call addIPToCache for as many times as the number of selectorItems matching the fqdn

@Dyanngg yes updated it.

Dyanngg · 2024-10-22T02:29:29Z

pkg/agent/controller/networkpolicy/fqdn.go

-	if mustCacheResponse {
+
+	if len(ipWithTTLMap) > 0 {
+		addressUpdate = true


I don't think this is correct. Isn't addressUpdate supposed to be determined by whether FQDN <-> IP mappings have changed?

@Dyanngg updated .

tnqn · 2024-10-22T02:18:05Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -405,80 +409,102 @@ func (f *fqdnController) deleteRuleSelectedPods(ruleID string) error {

 func (f *fqdnController) onDNSResponse(
 	fqdn string,
-	responseIPs map[string]net.IP,
-	lowestTTL uint32,
+	newDNSresponseIPs map[string]ipWithTTL,


In the camel format, it should be newDNSResponseIPs, however, I feel DNS and response is kind of duplicate with the function name, and given you name the struct ipWithTTL, I would just name the parameter newIPWithTTLs, or newIPWithTTLMap to correspond to the variable at L424.

tnqn · 2024-10-22T02:22:36Z

pkg/agent/controller/networkpolicy/fqdn.go

+	//minTimeToReQuery Establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
+	minTimeToReQuery := time.Now().Add(24 * time.Hour)


Suggested change

//minTimeToReQuery Establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.

minTimeToReQuery := time.Now().Add(24 * time.Hour)

// minTimeToRequery establishes a maximum reference time for tracking the minimum requery time to DNS, as IPs expire.

minTimeToRequery := time.Now().Add(24 * time.Hour)

tnqn · 2024-10-22T02:25:09Z

pkg/agent/controller/networkpolicy/fqdn.go

-					// This IP entry has already expired and not seen in the latest DNS response.
-					// It should be removed from the cache.
+
+		for cachedIpStr, cachedIpMeta := range cachedDNSMeta.responseIPs {


Suggested change

for cachedIpStr, cachedIpMeta := range cachedDNSMeta.responseIPs {

for cachedIPStr, cachedIPMeta := range cachedDNSMeta.responseIPs {

tnqn · 2024-10-22T02:28:39Z

pkg/agent/controller/networkpolicy/fqdn.go

-					// It should be removed from the cache.
+
+		for cachedIpStr, cachedIpMeta := range cachedDNSMeta.responseIPs {
+			if _, exist := newDNSresponseIPs[cachedIpStr]; !exist {


Suggested change

if _, exist := newDNSresponseIPs[cachedIpStr]; !exist {

if newIPMeta, exist := newDNSresponseIPs[cachedIpStr]; !exist {

then use it at L461, instead of querying the map another time.

tnqn · 2024-10-22T02:30:27Z

pkg/agent/controller/networkpolicy/fqdn.go

-	if mustCacheResponse {
+
+	if len(ipWithTTLMap) > 0 {
+		addressUpdate = true


when no IP is changed, I think the map is not empty, why this is set to true?

pkg/agent/controller/networkpolicy/fqdn.go

antoninbas · 2024-10-25T17:46:18Z

pkg/agent/controller/networkpolicy/fqdn.go

+	responseIPs map[string]ipWithTTL
+}
+
+type ipWithTTL struct {


this should be ipWithExpiration (we store the expiration time, not the TTL)

antoninbas · 2024-10-25T17:47:47Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -405,80 +408,115 @@ func (f *fqdnController) deleteRuleSelectedPods(ruleID string) error {

 func (f *fqdnController) onDNSResponse(
 	fqdn string,
-	responseIPs map[string]net.IP,
-	lowestTTL uint32,
+	newIPWithTTLMap map[string]ipWithTTL,


same comment: it should be newIPWithExpirationMap.

antoninbas · 2024-10-25T17:48:26Z

pkg/agent/controller/networkpolicy/fqdn.go

+	currentTime := time.Now()
+	ipWithTTLMap := make(map[string]ipWithTTL)
+
+	//minTimeToRequery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.


nit: leave whitespace after //

antoninbas · 2024-10-25T17:51:37Z

pkg/agent/controller/networkpolicy/fqdn.go

+				if !addressUpdate {
+					addressUpdate = true
+				}


typically, you just unconditionally write addressUpdate = true in this case. I'm pretty sure there is no advantage in checking the value first. If anything, the extra branching probably makes it worse.

antoninbas · 2024-10-25T17:53:35Z

pkg/agent/controller/networkpolicy/fqdn.go

+				if !addToCache {
+					addToCache = true
+				}


pkg/agent/controller/networkpolicy/fqdn.go

antoninbas · 2024-10-25T18:05:23Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+				if !exists {
+					t.Errorf("Expected %s to be found in dns cache.", ipStr)
+					continue
+				}
+
+				if cachedIPMeta.expirationTime.Before(time.Now()) {
+					t.Errorf("Expected %s to be found with a valid TTL,but it has expired.", ipStr)
+				}
+
+				if !cachedIPMeta.expirationTime.Equal(expectedTTL) {
+					t.Errorf("Expected TTL for %s to be %v, got %v", ipStr, expectedTTL, cachedIPMeta.expirationTime)
+				}


use testify assertions for consistency, not t.Error

pkg/agent/controller/networkpolicy/fqdn_test.go

antoninbas · 2024-10-25T18:09:49Z

test/e2e/framework.go

+	randomPatchAnnotationKey        = "test.antrea.io/random-value"
+	annotationValueLen              = 8
+	iperfPort                       = 5201
+	iperfSvcPort                    = 9999


I think I pointed this out before: please remove these unrelated changes from the PR

Dyanngg · 2024-10-28T02:44:20Z

pkg/agent/controller/networkpolicy/fqdn.go

+	//minTimeToRequery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
+	minTimeToRequery := currentTime.Add(24 * time.Hour)
+
+	addIPtoIPWithTTLMap := func(ip string, ipMeta ipWithTTL) {


some nits: addIPtoIPWithTTLMap -> updateMapWithIP, also minTimeToRequery -> timeToRequery as the former sounds like a duration rather than a timestamp

Dyanngg · 2024-10-28T02:52:21Z

pkg/agent/controller/networkpolicy/fqdn.go

 	} else {
+		// First time seeing this domain.
+		// check if this needs to be tracked, by checking its presence in the datapath rules.


Suggested change

// check if this needs to be tracked, by checking its presence in the datapath rules.

// check if this needs to be tracked, by checking if it matches any Antrea FQDN policy selectors.

Dyanngg · 2024-10-28T02:56:31Z

pkg/agent/controller/networkpolicy/fqdn.go

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


I'm not sure I understand this comment, but I'm pretty sure that it can be covered by the previous one

tnqn · 2024-10-29T02:05:00Z

pkg/agent/controller/networkpolicy/fqdn.go

+	// timeToRequery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
+	timeToRequery := currentTime.Add(24 * time.Hour)
+
+	updateMapWithIP := func(ip string, ipMeta ipWithExpiration) {
+		ipWithExpirationMap[ip] = ipMeta
+		if ipMeta.expirationTime.Before(timeToRequery) {
+			timeToRequery = ipMeta.expirationTime
+		}
+	}


Suggested change

// timeToRequery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.

timeToRequery := currentTime.Add(24 * time.Hour)

updateMapWithIP := func(ip string, ipMeta ipWithExpiration) {

ipWithExpirationMap[ip] = ipMeta

if ipMeta.expirationTime.Before(timeToRequery) {

timeToRequery = ipMeta.expirationTime

}

}

// timeToRequery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.

var timeToRequery *time.Time

updateMapWithIP := func(ip string, ipMeta ipWithExpiration) {

ipWithExpirationMap[ip] = ipMeta

if timeToRequery == nil || ipMeta.expirationTime.Before(*timeToRequery) {

timeToRequery = &ipMeta.expirationTime

}

}

To avoid introducing the vague duration 24 * time.Hour

Another way is to move least expiration calculation to a function of dnsMeta, then there is no need to have this function, and it can save a copy in "First time seeing this domain" case

This is awesome. I also had earlier thought on similar lines but i thought of it using as :

var timeToRequery time.Time

but as you know it meant timeToRequery gets set to its zero value and would not have worked because its zero value would have always been lesser, making me to set an arbitrary value to it.

tnqn · 2024-10-29T02:06:30Z

pkg/agent/controller/networkpolicy/fqdn.go

+	// timeToRequery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
+	timeToRequery := currentTime.Add(24 * time.Hour)
+
+	updateMapWithIP := func(ip string, ipMeta ipWithExpiration) {


Suggested change

updateMapWithIP := func(ip string, ipMeta ipWithExpiration) {

updateIPWithExpiration := func(ip string, ipMeta ipWithExpiration) {

tnqn · 2024-10-29T02:08:42Z

pkg/agent/controller/networkpolicy/fqdn.go

+		// check for presence of already cached IPs in the new response.
+		for cachedIPStr, cachedIPMeta := range cachedDNSMeta.responseIPs {
+			if newIPMeta, exist := newIPWithExpiration[cachedIPStr]; !exist {
+				// An already cached IP has been found in new response.


The comment says the opposite of the code.

tnqn · 2024-10-29T02:31:40Z

pkg/agent/controller/networkpolicy/fqdn_test.go

@@ -38,6 +39,12 @@ func newMockFQDNController(t *testing.T, controller *gomock.Controller, dnsServe
 	if dnsServer != nil {
 		dnsServerAddr = *dnsServer
 	}
+	mockDnsQueryQueue := workqueue.NewTypedRateLimitingQueueWithConfig(


Suggested change

mockDnsQueryQueue := workqueue.NewTypedRateLimitingQueueWithConfig(

mockDNSQueryQueue := workqueue.NewTypedRateLimitingQueueWithConfig(

tnqn · 2024-10-29T02:32:42Z

test/e2e/framework.go

+	randomPatchAnnotationKey        = "test.antrea.io/random-value"
+	annotationValueLen              = 8
+	iperfPort                       = 5201
+	iperfSvcPort                    = 9999


tnqn · 2024-10-29T02:35:24Z

Could you remove Draft PR for #6722 -- from the PR title? The issue link can be found in the PR description.

Also, it's perhaps time to squash the commits and write a good commit message.

tnqn · 2024-10-29T02:51:37Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+		{
+			name: "new IP added",
+			existingDNSCache: map[string]dnsMeta{
+				"fqdn-test-pod.lfx.test": {


It can use testFQDN.

pkg/agent/controller/networkpolicy/fqdn_test.go

tnqn · 2024-10-29T02:59:20Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			assert.Equal(t, len(cachedDnsMetaData.responseIPs), len(tc.expectedIPs), "Expected %d IPs in cache, got %d", len(tc.expectedIPs), len(cachedDnsMetaData.responseIPs))
+
+			for ipStr, expectedTTL := range tc.expectedIPs {
+				cachedIPMeta, exists := cachedDnsMetaData.responseIPs[ipStr]
+
+				assert.True(t, exists, "Expected %s to be found in dns cache.", ipStr)
+				assert.False(t, cachedIPMeta.expirationTime.Before(currentTime), "Expected %s to have a valid TTL, but it has expired.", ipStr)
+				assert.Equal(t, expectedTTL, cachedIPMeta.expirationTime, "Expected TTL for %s to be %v, got %v", ipStr, expectedTTL, cachedIPMeta.expirationTime)
+
+			}


you could just change tc.dnsResponseIPs to map[string]ipWithExpiration, then one line code can validate the result and cover all cases.

assert.Equal(t, tc.expectedIPs, cachedDNSMetaData.responseIPs)

tnqn · 2024-10-29T03:07:25Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+		workqueue.NewTypedItemExponentialFailureRateLimiter[string](1*time.Millisecond, 100*time.Millisecond),
+		workqueue.TypedRateLimitingQueueConfig[string]{
+			Name: "fqdn",
+		},


It should use a fake Clock to speed up enqueue, see

antrea/pkg/agent/controller/ipseccertificate/ipsec_certificate_controller_test.go

Line 280 in 47ce51e

fakeController := newFakeController(t, fakeClock)

tnqn · 2024-10-29T03:07:56Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+
+			}
+
+			if tc.expectedItem == testFQDN {


Suggested change

if tc.expectedItem == testFQDN {

if tc.expectedItem != "" {

…onses. - Implement tracking of individual IP TTLs for FQDNs in the DNS cache. - Upon expiration of an IP's TTL, trigger a new DNS query. - Evict only those IPs that are no longer present in the latest DNS response and have exceeded their original TTL. Signed-off-by: Hemant <hkbiet@gmail.com>

Dyanngg · 2024-10-29T19:03:24Z

pkg/agent/controller/networkpolicy/fqdn.go

 		}
-		f.dnsQueryQueue.AddAfter(fqdn, recordTTL.Sub(time.Now()))
+		f.dnsQueryQueue.AddAfter(fqdn, timeToReQuery.Sub(currentTime))


Please add a comment explaining why timeToReQuery cannot be nil here. Theoretically I think it cannot, but
it is not obvious at all

antoninbas · 2024-10-29T18:50:01Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+	fakeClock := clocktesting.NewFakeClock(time.Now())
+	currentTime := fakeClock.Now()


each test case should have its own clock, otherwise with your current code, one test advancing the clock will impact subsequent test cases

you can do something like this:

currentTime := time.Now() // use currentTime to define your test cases, as you are doing now // declare fakeClock := clocktesting.NewFakeClock(currentTime) at the beginning of each test case

antoninbas · 2024-10-29T18:53:13Z

pkg/agent/controller/networkpolicy/fqdn.go

+	currentTime := time.Now()
+	ipWithExpirationMap := make(map[string]ipWithExpiration)
+
+	// timeToReQuery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.


Suggested change

// timeToReQuery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.

// timeToRequery establishes the time after which a new DNS query should be sent for the FQDN.

// timeToRequery will be set to the minimum expiration time of all IPs added or updated in the cache.

suggesting this as I think the current comment is not very helpful or accurate

antoninbas · 2024-10-29T18:53:56Z

pkg/agent/controller/networkpolicy/fqdn.go

+	ipWithExpirationMap := make(map[string]ipWithExpiration)
+
+	// timeToReQuery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
+	var timeToReQuery *time.Time


nit: can we rename timeToReQuery to timeToRequery?

antoninbas · 2024-10-29T18:58:23Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			f, _ := newMockFQDNController(t, controller, nil)
+			mockDNSQueryQueue := workqueue.NewTypedRateLimitingQueueWithConfig(
+				workqueue.NewTypedItemExponentialFailureRateLimiter[string](minRetryDelay, maxRetryDelay),
+				workqueue.TypedRateLimitingQueueConfig[string]{
+					Name:  "fqdn-test",
+					Clock: fakeClock,
+				},
+			)
+			f.dnsQueryQueue = mockDNSQueryQueue
+			f.dnsEntryCache = tc.existingDNSCache


while this may work, a better approach is to have a clock.Clock as a field in fqdnController, and make sure the entire implementation of fqdnController uses that clock (e.g., calls clock.Now() instead of time.Now()). The default implementation will use clock.RealClock{}, but newMockFQDNController should support injecting a custom clock (fake clock).
with this approach, the test function won't need to build a custom workqueue

antoninbas · 2024-10-29T19:01:35Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			}
+
+			if tc.expectedItem != "" {
+				fakeClock.Step(5 * time.Second)


this 5 * time.Second values comes out of nowhere. I know it is based on the inputs to the test case (existingDNSCache, etc.) but it should itself be a part of the test case definition, otherwise it looks like some magical value

what I would do is remove tc.expectedItem and add tc.expectedRequeryAfter *time.Duration as a test case input. This way you only have one input for this. You already know that the expected item is testFQDN, so this doesn't need to be an input.

I'm also fixing a couple of issues in the snippet below:

if tc.expectedRequeryAfter != nil { fakeClock.Step(*tcexpectedRequeryAfter) // needed to avoid blocking on Get() in case of failure require.Eventually(t, func() bool { f.dnqQueryQueue.Len() > 0 }, 1*time.Second, 10*time.Millisecond) item, _ := f.dnsQueryQueue.Get() f.dnsQueryQueue.Done(item) assert.Equal(t, testFqdn, item) } else { // make sure that there is no requery assert.Never(t, func() bool { f.dnqQueryQueue.Len() > 0 }, 100*time.Millisecond, 10*time.Millisecond) }

Thank you for feedback and proper outline on this . I got some logical insights from this, for instance, I was directly calling Get() on the queue after advancing the time, without checking if the queue really contained any items or not because i "assumed" that it really must contain the item. This could have lead to blocking indefinitely until an item was actually available, and using require.Eventually helps ensure that the queue actually has items before attempting to retrieve them.

- Replace time.Now() usages with the clock set in fqdnController. - Inject fakeClock for tests. Removed mockDnsQueryQueue implementation. Signed-off-by: Hemant <hkbiet@gmail.com>

Dyanngg · 2024-10-29T21:53:22Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -17,7 +17,7 @@ package networkpolicy
 import (
 	"context"
 	"fmt"
-	"math"
+	"k8s.io/utils/clock"


This needs to be in the second import group

Dyanngg · 2024-10-29T21:54:21Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -152,6 +156,12 @@ type fqdnController struct {
 	ipv4Enabled           bool
 	ipv6Enabled           bool
 	gwPort                uint32
+	// clock allows for time-dependent operations within the fqdnController.
+	// By using a clock.Clock as a field, we ensure that all time-related calls throughout the
+	// implementation of the fqdnController utilize this clock, for example , calls to clock.Now() instead of time.Now()


Suggested change

// implementation of the fqdnController utilize this clock, for example , calls to clock.Now() instead of time.Now()

// implementation of the fqdnController utilize this clock

tnqn

"This patch enables the caching of TTLs for individual IPs in DNS responses." is not a good commit message title. Please follow the "git commit" link in https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-reviewers to write a proper one.

tnqn · 2024-10-30T15:44:54Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+		{
+			name: "existing DNS cache not impacted",
+			existingDNSCache: map[string]dnsMeta{
+				"fqdn-test-pod.lfx.test": {


Suggested change

"fqdn-test-pod.lfx.test": {

testFQDN: {

Same comment applies to other cases

tnqn · 2024-10-30T15:46:15Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			expectedIPs: map[string]ipWithExpiration{
+				"192.1.1.3": {ip: net.ParseIP("192.1.1.3"), expirationTime: currentTime.Add(5 * time.Second)},
+			},
+			expectedRequeryAfter: getDuration(5 * time.Second),


Suggested change

expectedRequeryAfter: getDuration(5 * time.Second),

expectedRequeryAfter: ptr.To(5 * time.Second),

and no need to create getDuration

tnqn · 2024-10-30T15:50:01Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			},
+		},
+		{
+			name: "old IP resent in DNS response is retained with an updated TTL fetched from response",


You could add another IP to existingDNSCache and dnsResponseIPs, the former has shorter expirationTime than the latter, to validate it's always the later expiration time is used.

pkg/agent/controller/networkpolicy/fqdn_test.go

…upport - Implement tracking of TTLs for individual IPs linked to FQDNs in the DNS cache. - Automatically trigger a new DNS query when an IP's TTL expires. - Evict only those IPs that are no longer present in the latest DNS response and have surpassed their original TTL. - Added clock field of type clock.Clock in fqdnController. - Pass clock while initializing fqdn controller. Signed-off-by: Hemant <hkbiet@gmail.com>

…antrea into cache-individual-ip-ttl

Signed-off-by: Hemant <hkbiet@gmail.com>

pkg/agent/controller/networkpolicy/fqdn.go

antoninbas · 2024-10-30T20:54:44Z

pkg/agent/controller/networkpolicy/fqdn.go

+		// timeToRequery passed below is guaranteed to be non-nil here because updateIPWithExpiration() is called on each IP in
+		// newIPWithExpiration that either isn't cached in the existing DNS cache or it remains unexpired. This ensures that timeToReQuery is
+		// always set to the earliest expiration time in the response, hence also enabling proper DNS re-query scheduling.


try to wrap lines to 100 chars at the max

pkg/agent/controller/networkpolicy/fqdn.go

pkg/agent/controller/networkpolicy/fqdn_test.go

pkg/agent/controller/networkpolicy/fqdn.go

…tuality test to 1 second Signed-off-by: Hemant <hkbiet@gmail.com>

pkg/agent/controller/networkpolicy/fqdn.go

pkg/agent/controller/networkpolicy/fqdn_test.go

pkg/agent/controller/networkpolicy/fqdn.go

…ueue. Updated onDNSResponse() to requeue DNS queries even when same IPs are receieved in a response. Added and updated comments as per review. Signed-off-by: Hemant <hkbiet@gmail.com>

Signed-off-by: Hemant <hkbiet@gmail.com>

pkg/agent/controller/networkpolicy/fqdn.go

pkg/agent/controller/networkpolicy/fqdn_test.go

Updated comment clarifying the case when ipWithExpirationMap is of zero length in onDNSResponse. Updated test case which ensures the requery time to be the later time amongst the TTL already cached and TTL received in DNS response for cases involving same IPs, as present in DNS cache, being received. Signed-off-by: Hemant <hkbiet@gmail.com>

pkg/agent/controller/networkpolicy/fqdn.go

antoninbas

only one small comment on my side, otherwise LGTM

pkg/agent/controller/networkpolicy/fqdn_test.go

pkg/agent/controller/networkpolicy/fqdn.go

regplaced dnsQueryQueue.AddAfter logic as before. Signed-off-by: Hemant <hkbiet@gmail.com>

tnqn

LGTM except a format issue

pkg/agent/controller/networkpolicy/fqdn.go

…ts from external imports Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas

LGTM

pkg/agent/controller/networkpolicy/fqdn_test.go

…NSResponse test. - Seemingly a race condition between the event of creation of Timer by DelayeingQueue and Step function of fakeClock is failing the test. - The fix Uses the idea in antrea-io#6407 - The fix uses the ,already implemented wrapper, around fakeClock with a method that counts the number of times DelayedQueue's Timer has been called. Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas

LGTM

pkg/agent/controller/networkpolicy/fqdn_test.go

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas

LGTM

antoninbas · 2024-11-05T19:13:13Z

/test-all

antoninbas reviewed Oct 11, 2024

View reviewed changes

antoninbas requested review from tnqn and Dyanngg October 11, 2024 22:31

hkiiita force-pushed the cache-individual-ip-ttl branch from d491609 to 5fbf0f1 Compare October 14, 2024 18:49

antoninbas reviewed Oct 14, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

tnqn reviewed Oct 15, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

Dyanngg reviewed Oct 17, 2024

View reviewed changes

antoninbas reviewed Oct 18, 2024

View reviewed changes

Dyanngg reviewed Oct 22, 2024

View reviewed changes

tnqn reviewed Oct 22, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

antoninbas reviewed Oct 25, 2024

View reviewed changes

Dyanngg reviewed Oct 28, 2024

View reviewed changes

tnqn reviewed Oct 29, 2024

View reviewed changes

hkiiita changed the title ~~Draft PR for #6722 -- cache TTLs for individual IP addresses in DNS responses.~~ cache TTLs for individual IP addresses in DNS responses. Oct 29, 2024

antoninbas marked this pull request as ready for review October 29, 2024 18:44

hkiiita force-pushed the cache-individual-ip-ttl branch from eb3f4ba to e98030f Compare October 29, 2024 18:49

Dyanngg reviewed Oct 29, 2024

View reviewed changes

antoninbas reviewed Oct 29, 2024

View reviewed changes

Added clock field of type clock.Clock in fqdnController type.

fe43d74

- Replace time.Now() usages with the clock set in fqdnController. - Inject fakeClock for tests. Removed mockDnsQueryQueue implementation. Signed-off-by: Hemant <hkbiet@gmail.com>

Dyanngg changed the title ~~cache TTLs for individual IP addresses in DNS responses.~~ Cache TTLs for individual IP addresses in DNS responses. Oct 29, 2024

Dyanngg reviewed Oct 29, 2024

View reviewed changes

tnqn reviewed Oct 30, 2024

View reviewed changes

hkiiita added 3 commits October 31, 2024 01:48

Merge branch 'cache-individual-ip-ttl' of https://github.com/hkiiita/…

d8fe685

…antrea into cache-individual-ip-ttl

group antrea imports

8b05b7e

Signed-off-by: Hemant <hkbiet@gmail.com>

fix imports using golangci-fix

2822742

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Oct 30, 2024

View reviewed changes

rephrase various comments as per review and correct poll time in even…

8a03d3a

…tuality test to 1 second Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Oct 30, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn_test.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn_test.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn.go Show resolved Hide resolved

hkiiita added 2 commits October 31, 2024 23:50

Refactored newFQDNController() to pass a clock to the DNS query workq…

cfcc0e0

…ueue. Updated onDNSResponse() to requeue DNS queries even when same IPs are receieved in a response. Added and updated comments as per review. Signed-off-by: Hemant <hkbiet@gmail.com>

correct grouping of imports in test file.

e3e25df

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Oct 31, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn.go Show resolved Hide resolved

tnqn reviewed Nov 1, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn_test.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/fqdn_test.go Outdated Show resolved Hide resolved

Dyanngg reviewed Nov 1, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

antoninbas reviewed Nov 1, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn_test.go Outdated Show resolved Hide resolved

antoninbas reviewed Nov 1, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

removed type assertion for clock parameter in test.

0753a33

regplaced dnsQueryQueue.AddAfter logic as before. Signed-off-by: Hemant <hkbiet@gmail.com>

luolanzone added this to the Antrea v2.2 release milestone Nov 4, 2024

tnqn reviewed Nov 4, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn.go Outdated Show resolved Hide resolved

Refactor import organization to separate local antrea.io/antrea impor…

c217871

…ts from external imports Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas previously approved these changes Nov 4, 2024

View reviewed changes

antoninbas reviewed Nov 4, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn_test.go Show resolved Hide resolved

hkiiita dismissed antoninbas’s stale review via 6c6e6f2 November 5, 2024 17:52

antoninbas reviewed Nov 5, 2024

View reviewed changes

pkg/agent/controller/networkpolicy/fqdn_test.go Outdated Show resolved Hide resolved

Refactor require.Eventually to a one-liner for consistency

f939732

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas approved these changes Nov 5, 2024

View reviewed changes

Dyanngg approved these changes Nov 5, 2024

View reviewed changes

antoninbas merged commit 232d582 into antrea-io:main Nov 5, 2024
56 of 57 checks passed

		// sample IP with time simulating expired time, i thought that using negative time will
		// simulate expired time as it will always equate to before when compared to any time.

		f.setFQDNMatchSelector(fqdn, selectorItem)
		for ipStr, ipMeta := range newDNSresponseIPs {

		//minTimeToReQuery Establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
		minTimeToReQuery := time.Now().Add(24 * time.Hour)

	for cachedIpStr, cachedIpMeta := range cachedDNSMeta.responseIPs {
	for cachedIPStr, cachedIPMeta := range cachedDNSMeta.responseIPs {

	if _, exist := newDNSresponseIPs[cachedIpStr]; !exist {
	if newIPMeta, exist := newDNSresponseIPs[cachedIpStr]; !exist {

	// check if this needs to be tracked, by checking its presence in the datapath rules.
	// check if this needs to be tracked, by checking if it matches any Antrea FQDN policy selectors.

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

	updateMapWithIP := func(ip string, ipMeta ipWithExpiration) {
	updateIPWithExpiration := func(ip string, ipMeta ipWithExpiration) {

	mockDnsQueryQueue := workqueue.NewTypedRateLimitingQueueWithConfig(
	mockDNSQueryQueue := workqueue.NewTypedRateLimitingQueueWithConfig(

		fakeClock := clocktesting.NewFakeClock(time.Now())
		currentTime := fakeClock.Now()

	// timeToReQuery establishes a maximum reference time for tracking the minimum re-query time to DNS, as IPs expire.
	// timeToRequery establishes the time after which a new DNS query should be sent for the FQDN.
	// timeToRequery will be set to the minimum expiration time of all IPs added or updated in the cache.

	// implementation of the fqdnController utilize this clock, for example , calls to clock.Now() instead of time.Now()
	// implementation of the fqdnController utilize this clock

	expectedRequeryAfter: getDuration(5 * time.Second),
	expectedRequeryAfter: ptr.To(5 * time.Second),

Cache TTLs for individual IP addresses in DNS responses. #6732

Cache TTLs for individual IP addresses in DNS responses. #6732

Conversation

hkiiita commented Oct 11, 2024

Choose a reason for hiding this comment

hkiiita Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

hkiiita Oct 18, 2024 •

edited

Loading

Dyanngg Oct 21, 2024 •

edited

Loading

tnqn commented Oct 29, 2024 •

edited

Loading

Dyanngg Oct 29, 2024 •

edited

Loading