[release-4.16] OCPBUGS-37078: Fix DNSNameResolver object status update issue

handler.go

@@ -199,14 +199,35 @@ func (resolver *OCPDNSNameResolver) updateResolvedNamesSuccess(
 		go func(namespace string, objName string) {
 			defer wg.Done()
 
+			previousResourceVersion := "0"
 			// Retry the update of the DNSNameResolver object if there's a conflict during the update.
 			retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-				// Fetch the DNSNameResolver object.
-				resolverObj, err := ocpnetworkv1alpha1lister.NewDNSNameResolverLister(
+				var (
+					resolverObj     *ocpnetworkapiv1alpha1.DNSNameResolver
+					resourceVersion string
+					err             error
+				)
+
+				// Fetch the DNSNameResolver object using the lister first.
+				resolverObj, err = ocpnetworkv1alpha1lister.NewDNSNameResolverLister(
 					resolver.dnsNameResolverInformer.GetIndexer()).DNSNameResolvers(namespace).Get(objName)
 				if err != nil {
 					return err
 				}
+				resourceVersion = resolverObj.GetResourceVersion()
+				// Check if the current and the previous resource version match or not.
+				if resourceVersion == previousResourceVersion {
+					listerResourceVersion := resourceVersion
+					// This indicates that there was a conflict and the lister has not caught up.
+					// So fetch the DNSNameResolver object using the client.
+					resolverObj, err = resolver.ocpNetworkClient.DNSNameResolvers(namespace).Get(ctx, objName, metav1.GetOptions{})
+					if err != nil {
+						return err
+					}
+					resourceVersion = resolverObj.GetResourceVersion()
+					log.Infof("lister was stale at resourceVersion=%v, live get showed resourceVersion=%v", listerResourceVersion, resourceVersion)
+				}
+				previousResourceVersion = resourceVersion
 
 				// Make a copy of the object. All the updates will be applied to the copied object.
 				newResolverObj := resolverObj.DeepCopy()
@@ -560,13 +581,34 @@ func (resolver *OCPDNSNameResolver) updateResolvedNamesFailure(ctx context.Conte
 		go func(namespace string, objName string) {
 			defer wg.Done()
 
+			previousResourceVersion := "0"
 			// Retry the update of the DNSNameResolver object if there's a conflict during the update.
 			retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-				// Fetch the DNSNameResolver object.
-				resolverObj, err := ocpnetworkv1alpha1lister.NewDNSNameResolverLister(resolver.dnsNameResolverInformer.GetIndexer()).DNSNameResolvers(namespace).Get(objName)
+				var (
+					resolverObj     *ocpnetworkapiv1alpha1.DNSNameResolver
+					resourceVersion string
+					err             error
+				)
+				// Fetch the DNSNameResolver object using the lister first.
+				resolverObj, err = ocpnetworkv1alpha1lister.NewDNSNameResolverLister(
+					resolver.dnsNameResolverInformer.GetIndexer()).DNSNameResolvers(namespace).Get(objName)
 				if err != nil {
 					return err
 				}
+				resourceVersion = resolverObj.GetResourceVersion()
+				// Check if the current and the previous resource version match or not.
+				if resourceVersion == previousResourceVersion {
+					listerResourceVersion := resourceVersion
+					// This indicates that there was a conflict and the lister has not caught up.
+					// So fetch the DNSNameResolver object using the client.
+					resolverObj, err = resolver.ocpNetworkClient.DNSNameResolvers(namespace).Get(ctx, objName, metav1.GetOptions{})
+					if err != nil {
+						return err
+					}
+					resourceVersion = resolverObj.GetResourceVersion()
+					log.Infof("lister was stale at resourceVersion=%v, live get showed resourceVersion=%v", listerResourceVersion, resourceVersion)
+				}
+				previousResourceVersion = resourceVersion
 
 				// Make a copy of the object. All the updates will be applied to the copied object.
 				newResolverObj := resolverObj.DeepCopy()

operator/controller/dnsnameresolver/resolver.go

@@ -105,23 +105,29 @@ func (resolver *Resolver) Start() {
 				nextDNSName, nextLookupTime, numIPs, exists = resolver.delete(deletedDNSDetails)
 			}
 			remainingDuration := time.Until(nextLookupTime)
-			if !exists || remainingDuration > defaultMaxTTL {
-				// If no DNS name is found OR If the remaining duration is greater than default maximum TTL, then perform DNS lookup
-				// after default maximum TTL.
-				timeTillNextLookup = defaultMaxTTL
-			} else if remainingDuration.Seconds() > 0 {
-				// If the remaining duration is positive and less than default maximum TTL, then perform DNS lookup
-				// after the remaining duration.
-				timeTillNextLookup = remainingDuration
-			} else {
-				// TTL of the DNS name has already expired, so send DNS lookup request as soon as possible.
-				timeTillNextLookup = 1 * time.Millisecond
-			}
+			timeTillNextLookup = getTimeTillNextLookup(exists, remainingDuration)
 			timer.Reset(timeTillNextLookup)
 		}
 	}()
 }
 
+// getTimeTillNextLookup returns the duration after which the next DNS lookup should be performed.
+func getTimeTillNextLookup(exists bool, remainingDuration time.Duration) time.Duration {
+	if !exists || remainingDuration > defaultMaxTTL {
+		// If no DNS name is found OR If the remaining duration is greater than default maximum TTL, then perform DNS lookup
+		// after default maximum TTL.
+		return defaultMaxTTL
+	} else if remainingDuration.Seconds() > 0 {
+		// If the remaining duration is positive and less than default maximum TTL, then perform DNS lookup
+		// after the remaining duration.
+		return remainingDuration
+	} else {
+		// A DNS lookup request has been sent upon TTL expiration of the DNS name. Reset the timer to wait until twice of default
+		// minimum TTL to perform the next lookup.
+		return 2 * defaultMinTTL
+	}
+}
+
 // AddResolvedName is called whenever a DNSNameResolver object is added or updated.
 func (resolver *Resolver) AddResolvedName(dnsDetails dnsDetails) {
 	// Send a signal to the added channel indicating that details corresponding to a DNS
@@ -304,6 +310,12 @@ func (resolver *Resolver) getNextDNSNameDetails() (string, time.Time, int, bool)
 			dns = dnsName
 			numIPs = resolvedName.numIPs
 		}
+		// If there are no IP addresses associated with the DNS name and the next lookup
+		// time of the DNS name is already past the current time, then reset the next
+		// lookup time to the default maximum TTL.
+		if resolvedName.numIPs == 0 && !time.Now().Before(resolvedName.minNextLookupTime) {
+			resolvedName.minNextLookupTime = time.Now().Add(defaultMaxTTL)
+		}
 	}
 	return dns, minNextLookupTime, numIPs, exists
 }

operator/controller/dnsnameresolver/resolver_test.go

@@ -232,6 +232,34 @@ func TestResolver(t *testing.T) {
 			expectedNumIPs:          []int{2, 2, 2, 1},
 			expectedOutputs:         []bool{true, true, true, true},
 		},
+		{
+			name: "Add a regular DNS name first with an IP address and with next lookup time past the current time," +
+				" then add the same regular DNS name again without any resolved address",
+			actions: []string{"Add", "Add"},
+			parameters: []interface{}{
+				&addParams{
+					dnsName: "www.example.com.",
+					resolvedAddresses: []networkv1alpha1.DNSNameResolverResolvedAddress{
+						{
+							IP:             "1.1.1.1",
+							TTLSeconds:     -5,
+							LastLookupTime: &v1.Time{Time: time.Now()},
+						},
+					},
+					matchesRegular: true,
+					objName:        "regular",
+				},
+				&addParams{
+					dnsName:        "www.example.com.",
+					matchesRegular: true,
+					objName:        "regular",
+				},
+			},
+			expectedNextDNSNames:    []string{"www.example.com.", "www.example.com."},
+			expectedNextLookupTimes: []time.Time{time.Now().Add(-5 * time.Second), time.Now().Add(defaultMaxTTL)},
+			expectedNumIPs:          []int{1, 0},
+			expectedOutputs:         []bool{true, true},
+		},
 	}
 
 	for _, tc := range tests {
@@ -278,3 +306,44 @@ func TestResolver(t *testing.T) {
 		})
 	}
 }
+
+func TestGetTimeTillNextLookup(t *testing.T) {
+	tests := []struct {
+		name                       string
+		dnsExists                  bool
+		remainingDuration          time.Duration
+		expectedTimeTillNextLookup time.Duration
+	}{
+		{
+			name:                       "DNS does not exist",
+			dnsExists:                  false,
+			remainingDuration:          0,
+			expectedTimeTillNextLookup: defaultMaxTTL,
+		},
+		{
+			name:                       "DNS exists and remaianing duration is greater than default max TTL",
+			dnsExists:                  true,
+			remainingDuration:          defaultMaxTTL + 1,
+			expectedTimeTillNextLookup: defaultMaxTTL,
+		},
+		{
+			name:                       "DNS exists and remaining duration is less than default max TTL",
+			dnsExists:                  true,
+			remainingDuration:          defaultMinTTL,
+			expectedTimeTillNextLookup: defaultMinTTL,
+		},
+		{
+			name:                       "DNS exists and remaining duration is not greater than 0",
+			dnsExists:                  true,
+			remainingDuration:          0,
+			expectedTimeTillNextLookup: 2 * defaultMinTTL,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			timeTillNextLookup := getTimeTillNextLookup(tc.dnsExists, tc.remainingDuration)
+			assert.Equal(t, tc.expectedTimeTillNextLookup, timeTillNextLookup)
+		})
+	}
+}

operator/go.mod

@@ -3,7 +3,6 @@ module github.com/openshift/coredns-ocp-dnsnameresolver/operator
 go 1.21
 
 require (
-	github.com/go-logr/logr v1.2.4
 	github.com/google/go-cmp v0.5.9
 	github.com/miekg/dns v1.1.58
 	github.com/onsi/ginkgo/v2 v2.13.0
@@ -22,9 +21,9 @@ require (
 	github.com/cespare/xxhash/v2 v2.2.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/emicklei/go-restful/v3 v3.11.0 // indirect
-	github.com/evanphx/json-patch v5.6.0+incompatible // indirect
 	github.com/evanphx/json-patch/v5 v5.8.0 // indirect
 	github.com/fsnotify/fsnotify v1.7.0 // indirect
+	github.com/go-logr/logr v1.2.4 // indirect
 	github.com/go-logr/zapr v1.2.4 // indirect
 	github.com/go-openapi/jsonpointer v0.19.6 // indirect
 	github.com/go-openapi/jsonreference v0.20.2 // indirect

operator/go.sum

@@ -63,6 +63,7 @@ github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+o
 github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
 github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
+github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
 github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
@@ -100,6 +101,7 @@ github.com/prometheus/common v0.45.0/go.mod h1:YJmSTw9BoKxJplESWWxlbyttQR4uaEcGy
 github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo=
 github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
 github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ=
+github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
 github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
@@ -119,6 +121,7 @@ github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1
 go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
 go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
 go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A=
+go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
 go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
 go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=
 go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
@@ -152,6 +155,7 @@ golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ=
+golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20191204072324-ce4227a45e2e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=