Merge branch 'main' into dk/1123-epp-cache

docs/architecture/decisions/0023-use-geventconnpool.md

@@ -0,0 +1,89 @@
+# 22. Use geventconnpool library for Connection Pooling
+
+Date: 2023-13-10
+
+## Status
+
+In Review
+
+## Context
+
+When sending and receiving data from the registry, we use the [EPPLib](https://github.com/cisagov/epplib) library to facilitate that process. To manage these connections within our application, we utilize a module named `epplibwrapper` which serves as a bridge between getgov and the EPPLib library. As part of this process, `epplibwrapper` will instantiate a client that handles sending/receiving data.
+
+At present, each time we need to send a command to the registry, the client establishes a new connection to handle this task. This becomes inefficient when dealing with multiple calls in parallel or in series, as we have to initiate a handshake for each of them. To mitigate this issue, a widely adopted solution is to use a [connection pool](https://en.wikipedia.org/wiki/Connection_pool). In general, a connection pool stores a cache of active connections so that rather than restarting the handshake process when it is unnecessary, we can utilize an existing connection to avoid this problem. 
+
+In practice, the lack of a connection pool has resulted in performance issues when dealing with connections to and from the registry. Given the unique nature of our development stack, our options for prebuilt libraries are limited. Out of our available options, a library called [`geventconnpool`](https://github.com/rasky/geventconnpool) was identified that most closely matched our needs. 
+
+## Considered Options
+
+**Option 1:** Use the existing connection pool library `geventconnpool`.
+<details open>
+<summary>➕ Pros</summary>
+
+- Saves development time and effort. 
+- A tiny library that is easy to audit and understand.
+- Built to be flexible, so every built-in function can be overridden with minimal effort.
+- This library has been used for [EPP before](https://github.com/rasky/geventconnpool/issues/9).
+- Uses [`gevent`](http://www.gevent.org/) for coroutines, which is reliable and well maintained.
+- [`gevent`](http://www.gevent.org/) is used in our WSGI web server.  
+- This library is the closest match to our needs that we have found.
+
+</details>
+<details open>
+<summary>➖ Cons</summary>
+
+- Not a well maintained library, could require a fork if a dependency breaks.
+- Heavily reliant on `gevent`.
+
+</details>
+
+**Option 2:** Write our own connection pool logic.
+<details open>
+<summary>➕ Pros</summary>
+
+- Full control over functionality, can be tailored to our specific needs.
+- Highly specific to our stack, could be fine tuned for performance.
+
+</details>
+<details open>
+<summary>➖ Cons</summary>
+
+- Requires significant development time and effort, needs thorough testing.
+- Would require managing with and developing around concurrency.
+- Introduces the potential for many unseen bugs.
+
+</details>
+
+**Option 3:** Modify an existing library which we will then tailor to our needs.
+<details open>
+<summary>➕ Pros</summary>
+
+- Savings in development time and effort, can be tailored to our specific needs.
+- Good middleground between the first two options.
+
+</details>
+<details open>
+<summary>➖ Cons</summary>
+
+- Could introduce complexity, potential issues with maintaining the modified library.
+- May not be necessary if the given library is flexible enough. 
+
+</details>
+
+## Decision
+
+We have decided to go with option 1, which is to use the `geventconnpool` library. It closely matches our needs and offers several advantages. Of note, it significantly saves on development time and it is inherently flexible. This allows us to easily change functionality with minimal effort. In addition, the gevent library (which this uses) offers performance benefits due to it being a) written in [cython](https://cython.org/), b) very well maintained and purpose built for tasks such as these, and c) used in our WGSI server. 
+
+In summary, this decision was driven by the library's flexibility, simplicity, and compatibility with our tech stack. We acknowledge the risk associated with its maintenance status, but believe that the benefit outweighs the risk. 
+
+## Consequences
+
+While its small size makes it easy to work around, `geventconnpool` is not actively maintained. Its last update was in 2021, and as such there is a risk that its dependencies (gevent) will outpace this library and cause it to break. If such an event occurs, it would require that we fork the library and fix those issues. See option 3 pros/cons.
+
+## Mitigation Plan
+To manage this risk, we'll:
+
+1. Monitor the gevent library for updates.
+2. Design the connection pool logic abstractly such that we can easily swap the underlying logic out without needing (or minimizing the need) to rewrite code in `epplibwrapper`.
+3. Document a process for forking and maintaining the library if it becomes necessary, including testing procedures.
+4. Establish a contingency plan for reverting to a previous system state or switching to a different library if significant issues arise with `gevent` or `geventconnpool`.

src/epplibwrapper/__init__.py

@@ -45,7 +45,7 @@
 # Attn: these imports should NOT be at the top of the file
 try:
     from .client import CLIENT, commands
-    from .errors import RegistryError, ErrorCode
+    from .errors import RegistryError, ErrorCode, CANNOT_CONTACT_REGISTRY, GENERIC_ERROR
     from epplib.models import common, info
     from epplib.responses import extensions
     from epplib import responses
@@ -61,4 +61,6 @@
     "info",
     "ErrorCode",
     "RegistryError",
+    "CANNOT_CONTACT_REGISTRY",
+    "GENERIC_ERROR",
 ]

src/epplibwrapper/errors.py

@@ -1,5 +1,8 @@
 from enum import IntEnum
 
+CANNOT_CONTACT_REGISTRY = "Update failed. Cannot contact the registry."
+GENERIC_ERROR = "Value entered was wrong."
+
 
 class ErrorCode(IntEnum):
     """

src/registrar/models/domain.py

@@ -817,7 +817,7 @@ def _set_singleton_contact(self, contact: PublicContact, expectedType: str):  #
             and errorCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY
         ):
             # TODO- ticket #433 look here for error handling
-            raise Exception("Unable to add contact to registry")
+            raise RegistryError(code=errorCode)
 
         # contact doesn't exist on the domain yet
         logger.info("_set_singleton_contact()-> contact has been added to the registry")

src/registrar/tests/common.py

@@ -33,6 +33,8 @@
     ErrorCode,
 )
 
+from registrar.models.utility.contact_error import ContactError, ContactErrorCodes
+
 logger = logging.getLogger(__name__)
 
 
@@ -793,14 +795,8 @@ def mockSend(self, _request, cleaned):
             return self.mockInfoContactCommands(_request, cleaned)
         elif isinstance(_request, commands.UpdateDomain):
             return self.mockUpdateDomainCommands(_request, cleaned)
-        elif (
-            isinstance(_request, commands.CreateContact)
-            and getattr(_request, "id", None) == "fail"
-            and self.mockedSendFunction.call_count == 3
-        ):
-            # use this for when a contact is being updated
-            # sets the second send() to fail
-            raise RegistryError(code=ErrorCode.OBJECT_EXISTS)
+        elif isinstance(_request, commands.CreateContact):
+            return self.mockCreateContactCommands(_request, cleaned)
         elif isinstance(_request, commands.CreateHost):
             return MagicMock(
                 res_data=[self.mockDataHostChange],
@@ -897,6 +893,24 @@ def mockInfoContactCommands(self, _request, cleaned):
 
         return MagicMock(res_data=[mocked_result])
 
+    def mockCreateContactCommands(self, _request, cleaned):
+        if (
+            getattr(_request, "id", None) == "fail"
+            and self.mockedSendFunction.call_count == 3
+        ):
+            # use this for when a contact is being updated
+            # sets the second send() to fail
+            raise RegistryError(code=ErrorCode.OBJECT_EXISTS)
+        elif getattr(_request, "email", None) == "[email protected]":
+            # use this for when a contact is being updated
+            # mocks a registry error on creation
+            raise RegistryError(code=None)
+        elif getattr(_request, "email", None) == "[email protected]":
+            # use this for when a contact is being updated
+            # mocks a contact error on creation
+            raise ContactError(code=ContactErrorCodes.CONTACT_TYPE_NONE)
+        return MagicMock(res_data=[self.mockDataInfoHosts])
+
     def setUp(self):
         """mock epp send function as this will fail locally"""
         self.mockSendPatch = patch("registrar.models.domain.registry.send")

src/registrar/tests/test_views.py

@@ -1554,6 +1554,78 @@ def test_domain_security_email_form(self):
             success_page, "The security email for this domain has been updated"
         )
 
+    def test_security_email_form_messages(self):
+        """
+        Test against the success and error messages that are defined in the view
+        """
+        p = "adminpass"
+        self.client.login(username="superuser", password=p)
+
+        form_data_registry_error = {
+            "security_email": "[email protected]",
+        }
+
+        form_data_contact_error = {
+            "security_email": "[email protected]",
+        }
+
+        form_data_success = {
+            "security_email": "[email protected]",
+        }
+
+        test_cases = [
+            (
+                "RegistryError",
+                form_data_registry_error,
+                "Update failed. Cannot contact the registry.",
+            ),
+            ("ContactError", form_data_contact_error, "Value entered was wrong."),
+            (
+                "RegistrySuccess",
+                form_data_success,
+                "The security email for this domain has been updated.",
+            ),
+            # Add more test cases with different scenarios here
+        ]
+
+        for test_name, data, expected_message in test_cases:
+            response = self.client.post(
+                reverse("domain-security-email", kwargs={"pk": self.domain.id}),
+                data=data,
+                follow=True,
+            )
+
+            # Check the response status code, content, or any other relevant assertions
+            self.assertEqual(response.status_code, 200)
+
+            # Check if the expected message tag is set
+            if test_name == "RegistryError" or test_name == "ContactError":
+                message_tag = "error"
+            elif test_name == "RegistrySuccess":
+                message_tag = "success"
+            else:
+                # Handle other cases if needed
+                message_tag = "info"  # Change to the appropriate default
+
+            # Check the message tag
+            messages = list(response.context["messages"])
+            self.assertEqual(len(messages), 1)
+            message = messages[0]
+            self.assertEqual(message.tags, message_tag)
+            self.assertEqual(message.message, expected_message)
+
+    def test_domain_overview_blocked_for_ineligible_user(self):
+        """We could easily duplicate this test for all domain management
+        views, but a single url test should be solid enough since all domain
+        management pages share the same permissions class"""
+        self.user.status = User.RESTRICTED
+        self.user.save()
+        home_page = self.app.get("/")
+        self.assertContains(home_page, "igorville.gov")
+        with less_console_noise():
+            response = self.client.get(reverse("domain", kwargs={"pk": self.domain.id}))
+            self.assertEqual(response.status_code, 403)
+
 
 class TestDomainDNSSEC(TestDomainOverview):
 

src/registrar/views/domain.py

@@ -23,6 +23,7 @@
     UserDomainRole,
 )
 from registrar.models.public_contact import PublicContact
+from registrar.models.utility.contact_error import ContactError
 
 from ..forms import (
     ContactForm,
@@ -41,6 +42,8 @@
     common,
     extensions,
     RegistryError,
+    CANNOT_CONTACT_REGISTRY,
+    GENERIC_ERROR,
 )
 
 from ..utility.email import send_templated_email, EmailSendingError
@@ -626,15 +629,27 @@ def form_valid(self, form):
         # If no default is created for security_contact,
         # then we cannot connect to the registry.
         if contact is None:
-            messages.error(self.request, "Update failed. Cannot contact the registry.")
+            messages.error(self.request, CANNOT_CONTACT_REGISTRY)
             return redirect(self.get_success_url())
 
         contact.email = new_email
-        contact.save()
 
-        messages.success(
-            self.request, "The security email for this domain has been updated."
-        )
+        try:
+            contact.save()
+        except RegistryError as Err:
+            if Err.is_connection_error():
+                messages.error(self.request, CANNOT_CONTACT_REGISTRY)
+                logger.error(f"Registry connection error: {Err}")
+            else:
+                messages.error(self.request, GENERIC_ERROR)
+                logger.error(f"Registry error: {Err}")
+        except ContactError as Err:
+            messages.error(self.request, GENERIC_ERROR)
+            logger.error(f"Generic registry error: {Err}")
+        else:
+            messages.success(
+                self.request, "The security email for this domain has been updated."
+            )
 
         # superclass has the redirect
         return redirect(self.get_success_url())