fix: Requeue DNSEndpoint request when create fails with already exists error (nginx#3637)

* Requeue DNSEndpoint when create fails with already exists error

* Add python tests

Author: ciarams87

internal/externaldns/sync.go

@@ -44,7 +44,7 @@ func SyncFnFor(rec record.EventRecorder, client clientset.Interface, ig map[stri
 		}
 
 		if vs.Status.ExternalEndpoints == nil {
-			// It can take time for the external endpoints to sync
+			// It can take time for the external endpoints to sync - kick it back to the queue
 			glog.V(3).Info("Failed to determine external endpoints - retrying")
 			return fmt.Errorf("failed to determine external endpoints")
 		}
@@ -60,7 +60,7 @@ func SyncFnFor(rec record.EventRecorder, client clientset.Interface, ig map[stri
 
 		newDNSEndpoint, updateDNSEndpoint, err := buildDNSEndpoint(nsi.extdnslister, vs, targets, recordType)
 		if err != nil {
-			glog.Errorf("error message here %s", err)
+			glog.Errorf("incorrect DNSEndpoint config for VirtualServer resource: %s", err)
 			rec.Eventf(vs, corev1.EventTypeWarning, reasonBadConfig, "Incorrect DNSEndpoint config for VirtualServer resource: %s", err)
 			return err
 		}
@@ -72,6 +72,11 @@ func SyncFnFor(rec record.EventRecorder, client clientset.Interface, ig map[stri
 			glog.V(3).Infof("Creating DNSEndpoint for VirtualServer resource: %v", vs.Name)
 			dep, err = client.ExternaldnsV1().DNSEndpoints(newDNSEndpoint.Namespace).Create(ctx, newDNSEndpoint, metav1.CreateOptions{})
 			if err != nil {
+				if apierrors.IsAlreadyExists(err) {
+					// Another replica likely created the DNSEndpoint since we last checked - kick it back to the queue
+					glog.V(3).Info("DNSEndpoint has been created since we last checked - retrying")
+					return fmt.Errorf("DNSEndpoint has already been created")
+				}
 				glog.Errorf("Error creating DNSEndpoint for VirtualServer resource: %v", err)
 				rec.Eventf(vs, corev1.EventTypeWarning, reasonBadConfig, "Error creating DNSEndpoint for VirtualServer resource %s", err)
 				return err
@@ -175,7 +180,7 @@ func buildDNSEndpoint(extdnsLister extdnslisters.DNSEndpointLister, vs *vsapi.Vi
 	vs = vs.DeepCopy()
 
 	if existingDNSEndpoint != nil {
-		glog.V(3).Infof("DNDEndpoint already exist for this object, ensuring it is up to date")
+		glog.V(3).Infof("DNSEndpoint already exists for this object, ensuring it is up to date")
 		if metav1.GetControllerOf(existingDNSEndpoint) == nil {
 			glog.V(3).Infof("DNSEndpoint has no owner. refusing to update non-owned resource")
 			return nil, nil, nil

tests/suite/test_virtual_server_externaldns.py

@@ -1,7 +1,7 @@
 import pytest
 from settings import TEST_DATA
-from suite.utils.custom_assertions import assert_event
-from suite.utils.custom_resources_utils import is_dnsendpoint_present
+from suite.utils.custom_assertions import assert_event, assert_event_not_present
+from suite.utils.custom_resources_utils import is_dnsendpoint_present, read_custom_resource
 from suite.utils.resources_utils import get_events, patch_namespace_with_label, wait_before_test
 from suite.utils.vs_vsr_resources_utils import patch_virtual_server_from_yaml
 from suite.utils.yaml_utils import get_name_from_yaml, get_namespace_from_yaml
@@ -48,6 +48,21 @@ def test_responses_after_setup(
             wait_before_test(1)
             print(f"External DNS not updated, retrying... #{retry}")
         assert wanted_string in log_contents
+        print("\nStep 3: Verify VS status is Valid and no bad config events occurred")
+        events = get_events(kube_apis.v1, virtual_server_setup.namespace)
+        vs_bad_config_event = "Error creating DNSEndpoint for VirtualServer resource"
+        assert_event_not_present(vs_bad_config_event, events)
+        response = read_custom_resource(
+            kube_apis.custom_objects,
+            virtual_server_setup.namespace,
+            "virtualservers",
+            virtual_server_setup.vs_name,
+        )
+        assert (
+            response["status"]
+            and response["status"]["reason"] == "AddedOrUpdated"
+            and response["status"]["state"] == "Valid"
+        )
 
     def test_update_to_ed_in_vs(
         self, kube_apis, crd_ingress_controller_with_ed, create_externaldns, virtual_server_setup
@@ -65,6 +80,18 @@ def test_update_to_ed_in_vs(
         wait_before_test(5)
         events = get_events(kube_apis.v1, virtual_server_setup.namespace)
         assert_event(vs_event_update_text, events)
+        print("\nStep 3: Verify VS status is Valid")
+        response = read_custom_resource(
+            kube_apis.custom_objects,
+            virtual_server_setup.namespace,
+            "virtualservers",
+            virtual_server_setup.vs_name,
+        )
+        assert (
+            response["status"]
+            and response["status"]["reason"] == "AddedOrUpdated"
+            and response["status"]["state"] == "Valid"
+        )
 
 
 @pytest.mark.vs
@@ -122,3 +149,18 @@ def test_responses_after_setup(
             wait_before_test(1)
             print(f"External DNS not updated, retrying... #{retry}")
         assert wanted_string in log_contents
+        print("\nStep 3: Verify VS status is Valid and no bad config events occurred")
+        events = get_events(kube_apis.v1, virtual_server_setup.namespace)
+        vs_bad_config_event = "Error creating DNSEndpoint for VirtualServer resource"
+        assert_event_not_present(vs_bad_config_event, events)
+        response = read_custom_resource(
+            kube_apis.custom_objects,
+            virtual_server_setup.namespace,
+            "virtualservers",
+            virtual_server_setup.vs_name,
+        )
+        assert (
+            response["status"]
+            and response["status"]["reason"] == "AddedOrUpdated"
+            and response["status"]["state"] == "Valid"
+        )

tests/suite/utils/custom_assertions.py

@@ -143,6 +143,19 @@ def assert_event(event_text, events_list) -> None:
     pytest.fail(f'Failed to find the event "{event_text}" in the list. Exiting...')
 
 
+def assert_event_not_present(event_text, events_list) -> None:
+    """
+    Search for the event in the list.
+
+    :param event_text: event text
+    :param events_list: list of events
+    :return:
+    """
+    for i in range(len(events_list) - 1, -1, -1):
+        if event_text in events_list[i].message:
+            pytest.fail(f'Event "{event_text}" exists in the list. Exiting...')
+
+
 def assert_event_starts_with_text_and_contains_errors(event_text, events_list, fields_list) -> None:
     """
     Search for the event starting with the expected text in the list and check its message.