Redesign communication client types #409

robertbartel · 2023-08-07T17:12:30Z

Separation of transport layer client duties to separate type(s), and moving websocket client to this hierarchy
Separation of auth to separate types
Moving to more composition and less strict type hinting in service clients

robertbartel · 2023-08-07T17:14:20Z

This is still very much a work in progress, and I haven't even committed all the changes I've been making (just those specifically in client.py so far). I wanted to get this available to look at though, to show the general direction(s) I am headed in.

Redesigning and renaming abstraction, making it more focused on just the transport layer communication handling, rather than general service client behavior; also extending new type with a subtype that adds SSL context security. Making RequestClient concrete but generic. Add ConnectionContextClient as an abstract transport client than maintains a connection via an async managed context, and (re)implementing WebSocketClient using the new (secure) transport client with a context managed connection. Update specialized clients to extend RequestClient Fix ExternalClient with changes.

Deprecating to help move away from service-specific types in a general library.

Updating handlers to account for changes to communication package.

robertbartel · 2023-08-10T20:20:57Z

The remaining failing tests are in the dmod.client package. That also will have some significant changes coming soon, beyond just what is needed to remedy the failures. Unless there are any objections, I am going to keep those changes separate and give them their own PR.

aaraney

In general I think this is a great step in an improved direction! Thanks for reworking this @robertbartel. I left several comments that will require a bit of conversation, but once we've worked through that, we should be on the road. Thanks!

aaraney · 2023-08-15T17:54:51Z

python/lib/communication/dmod/communication/client.py

@@ -115,135 +104,277 @@ async def async_recv(self) -> str:
        """
        pass

+
+class SSLSecuredTransportLayerClient(TransportLayerClient, ABC):


What are your thoughts on removing this and injecting an optional ssl_context in TransportLayerClient's initializer?

Something like:

def __init__(self, endpoint_uri: str, *args, ssl_context: Optional[ssl.SSLContext] = None, **kwargs) ): ...

We could then just provide a helper function to create an ssl context from a directory. So pretty much what client_ssl_context does. Thoughts?

After thinking a little about it, I'd prefer not to accept an SSLContext directly. I'm splitting hairs a bit, but this strictly speaking would increase the ways we could end up with an improperly/incompletely initialized context in the client. If we did that, I would want us to somehow ensure that the _SSLMethod was right and that the context had right the CA certs loaded. I think its easier to just be responsible for creating the context object.

We could accept an optional SSL directory as part of the TransportLayerClient's initializer and create the context there instead of the lazy property. Then we could still remove SSLSecuredTransportLayerClient. What are your thoughts?

I think that is a fair tradeoff, introducing a little complexity to avoid misuse. My only request would be to provide a way to pass a default ssl context.

I've updated things to optionally bundle the SSL context into TransportLayerClient, remove SSLSecuredTransportLayerClient, and parameterize whether to use a default context.

python/lib/communication/dmod/communication/_version.py

python/lib/communication/dmod/communication/client.py

aaraney · 2023-08-17T16:40:05Z

python/lib/communication/dmod/communication/client.py

+                    msg = f'********** {self.__class__.__name__} could not deserialize {response_type.__name__} ' \
+                          f'from raw websocket response: `{str(response_str)}`'
+                    reason = f'{self.__class__.__name__} Could Not Deserialize To {response_type.__name__}'
+                    response_object = response_type(success=False, reason=reason, message=msg, data=response_json)


I really wish that we had algebraic datatypes in python, I think that concept would be really useful here. My main concern is returning client side errors as if they are response objects from the server. Meaning, there is no semantic distinction between a server sent failed response and a response the client couldnt handle.

Like you have it written, I dont think we should throw here, but I do think we should either return a tuple of an optional response and an optional client side error or return either a response or a client side error type. Thoughts?

I’m not entirely certain we shouldn’t throw here. It is fundamentally an exception to what the client can handle. To properly address it that way, we’d need a (or a few) custom exception type(s) for indicating the specific problem(s). And then we'd need to handle the exception properly somewhere else further up the stack.

I'm not definitively sold on that as a solution, though. My first impression is that it'd be a little less clean, but I'd consider a solution that involved returning a tuple.

Regardless, addressing this properly would be fairly intricate, and what's in place now was carried over from previous code, albeit perhaps arranged slightly differently. I.e., it's out of scope for this PR, but it deserves its own issue.

And then we'd need to handle the exception properly somewhere else further up the stack.

Right, this is my primary motivation for trying to treat the exceptions as values instead of throwing here. Treating them as values without attaching for example, a traceback, could also become difficult to debug. I personally think it is worth the headache though. Exceptions are so easy to ignore / mishandle.

Regardless, addressing this properly would be fairly intricate, and what's in place now was carried over from previous code, albeit perhaps arranged slightly differently. I.e., it's out of scope for this PR, but it deserves its own issue.

Also totally fair, thanks for calling out of the scope creep!

FYI, I've opened #418 for this.

aaraney · 2023-08-17T17:01:39Z

python/lib/communication/dmod/communication/client.py

+        response_json = {}
+        try:
+            # Send the request and get the service response
+            serialized_response = await self._transport_client.async_send(data=str(message), await_response=True)


Ordering is not guaranteed here so you could get a response message for a different request or different request type all together. At the moment, I dont think that is the case given how we are using websockets currently, but this is certainly something that we will need to prioritize solving soon.

Correct. I'm still considering whether to attempt to address that here or in a subsequent PR, and how to accomplish that.

Thanks for letting me know where you are with this. JLMK if you plan to address it in a separate PR, for now I will treat it like you are going to address it here.

I am addressing this is a separate PR: #417.

python/lib/communication/dmod/communication/client.py

Making this a static method rather than a class method, as it doesn't need any class details.

Moving to top-level interface rather than having subtype for this.

Removing redundant SSLSecuredTransportLayerClient subtype.

Removing redundant SSLSecuredTransportLayerClient from communication package __init__.py.

Modifying TransportLayerClient to expect init params for (most of) the components of a URI, rather than the endpoint URI directly, and adding abstract property so that concrete types can implement this themselves (e.g., to control the specific protocol component used in the URI).

aaraney

Thanks for working through this, @robertbartel! I left a few comments that mainly address some usability issues. After we work through that, this should be good to go. Thanks!

aaraney · 2023-09-08T17:05:01Z

python/lib/communication/dmod/communication/client.py

+        """
+        pass
+
+    def __init__(self, endpoint_host: str, endpoint_port: Union[int, str], endpoint_path: Optional[str] = None,


This feels like it is doing a lot, but I think it is alright. Thanks for reworking this @robertbartel! My only comment is, thoughts on making all arguments required keyword arguments? So the signature would change to:

def __init__( self, *, endpoint_host: str, endpoint_port: Union[int, str], endpoint_path: Optional[str] = None, cafile: Optional[Path] = None, capath: Optional[Path] = None, use_default_context: bool = False, **kwargs ):

Sure, I've just made and pushed that change.

aaraney · 2023-09-08T17:07:40Z

python/lib/communication/dmod/communication/client.py

        """
        pass

+    @property
+    def client_ssl_context(self) -> Optional[ssl.SSLContext]:


Does this need to be public?

I don't think so, no. I went ahead and changed that as well.

python/lib/communication/dmod/communication/client.py

aaraney · 2023-09-08T17:13:32Z

python/lib/communication/dmod/communication/client.py

@@ -117,133 +161,266 @@ async def async_recv(self) -> str:

    @property
    @abstractmethod
-    def client_ssl_context(self) -> ssl.SSLContext:
+    def endpoint_uri(self) -> str:


Does this need to be public? Just trying to reduce the public interface to only what is required.

I'll change this also for now, but it (or something close to it) may have to come back for #417. My reasoning there may have actually been circular, though, so maybe it is superfluous.

aaraney · 2023-09-08T17:18:58Z

python/lib/communication/dmod/communication/client.py

    @property
-    def is_new_session(self):
-        return self._is_new_session
+    def force_reload(self) -> bool:


This looks like it does not need to be reimplemented in the subclass. Am I missing something?

Not sure what you mean. This is being introduced by this class (CachedAuthClient). There is no previous declaration/definition.

aaraney · 2023-09-08T17:30:12Z

python/lib/communication/dmod/communication/client.py

-        return '{}://{}:{}{}'.format(proto, host.strip(), str(port).strip(), path)
-
-    def __init__(self, ssl_directory: Path, *args, **kwargs):
+    def __init__(self, transport_client: TransportLayerClient, default_response_type: Optional[Type[Response]] = None,


I think its a good idea to make these keyword only too. Its probably best that we do that with all of these classes given that we have a fair amount of inheritance.

Sure, I've made that change too.

Cleaning up TransportLayerClient __init__ to not pass var/keyword args to super (ABC), and removing abstract annotation from implemented method in WebSocketClient.

Fixing setup of scheduler client unit tests by adding implementation of this classmethod, declared as abstract in upstream superclass.

Removing public property in TransportLayerClient and replacing with use of private attribute.

Clarify that endpoint_host does not include protocol in docstring. Co-authored-by: Austin Raney <austin.raney@noaa.gov>

aaraney

Thanks for making the changes, @robertbartel! Merge at will!

robertbartel added enhancement New feature or request maas MaaS Workstream labels Aug 7, 2023

robertbartel requested a review from aaraney August 7, 2023 17:12

robertbartel added 3 commits August 9, 2023 11:42

Add AuthClient and CachedAuthClient.

733ab9d

Modify ExternalRequestClient to use AuthClient

3ef01cf

robertbartel force-pushed the f/cli_updates/client_refactor branch from 15271be to 3ef01cf Compare August 9, 2023 16:42

robertbartel added 7 commits August 10, 2023 13:29

Remove obsolete ExternalClient.

6b7c6f1

Un-generifying RequestClient and subtypes.

52dfa9f

Updating tests for client changes.

072d086

Updating comms package init.

7362f99

Bump comms package to 1.0.0.

7e80278

Deprecating some RequestClient subclasses.

7050ff6

Deprecating to help move away from service-specific types in a general library.

Remove obsoleted generic type vars.

f56bc98

robertbartel marked this pull request as ready for review August 10, 2023 19:11

robertbartel added 3 commits August 10, 2023 14:38

Update externalrequests to latest comms dep.

5b6d015

Update external request handlers for comms.

fbf6fa0

Updating handlers to account for changes to communication package.

Bump externalrequests package version to 0.5.0.

3f421ab

aaraney requested changes Aug 17, 2023

View reviewed changes

robertbartel added 9 commits August 21, 2023 14:54

Adjusting comms version to not be 1.0.0 yet.

2bc8fc3

Adjusting comms dep to not be 1.0.0 yet.

f05a09c

Fix optional client session property type hints.

97548fa

For cached auth, use fix, common default basename.

b7ac988

Make WebSocketClient.build_endpoint_uri static.

4be3cb1

Making this a static method rather than a class method, as it doesn't need any class details.

Put SSLContext directly in TransportLayerClient.

9e60aa0

Moving to top-level interface rather than having subtype for this.

Remove SSLSecuredTransportLayerClient.

abd58db

Removing redundant SSLSecuredTransportLayerClient subtype.

Remove SSLSecuredTransportLayerClient from init.

da0c407

Removing redundant SSLSecuredTransportLayerClient from communication package __init__.py.

Refactor WebSocketClient after SSL client removal.

5b6160d

robertbartel added 3 commits August 25, 2023 15:05

Update request handlers for client class changes.

2815193

Update comms mock test class for client changes.

b3ec89e

This was referenced Aug 25, 2023

Transport Client Multiplexer #417

Draft

Improve handling of different failure scenarios for message send and response receive #418

Open

Tweak transport client endpoint uri handling.

5d834f4

aaraney requested changes Sep 8, 2023

View reviewed changes

robertbartel added 2 commits September 8, 2023 15:37

Fix issues in client.py.

3bc4cc6

Cleaning up TransportLayerClient __init__ to not pass var/keyword args to super (ABC), and removing abstract annotation from implemented method in WebSocketClient.

Fix MockTransportLayerClient for unit tests.

828a1ea

Fixing setup of scheduler client unit tests by adding implementation of this classmethod, declared as abstract in upstream superclass.

robertbartel mentioned this pull request Sep 8, 2023

Improve dmod.client and update CLI #424

Merged

robertbartel and others added 6 commits September 8, 2023 15:55

Have transport client init only use keyword args.

4dce763

Remove public client_ssl_context property.

6d63be3

Removing public property in TransportLayerClient and replacing with use of private attribute.

Refactor endpoint_uri property to private func.

583dbf3

Clarify that endpoint_host does not include protocol.

0aaf94a

Clarify that endpoint_host does not include protocol in docstring. Co-authored-by: Austin Raney <austin.raney@noaa.gov>

Update RequestClient init to keyword args only.

a1bbb5e

Update scheduler client tests for endpoint uri.

92e58e8

aaraney approved these changes Sep 11, 2023

View reviewed changes

robertbartel merged commit 9f964e1 into NOAA-OWP:master Sep 12, 2023

robertbartel deleted the f/cli_updates/client_refactor branch September 13, 2023 18:35

aaraney mentioned this pull request Nov 2, 2023

ModelExecRequestClient removed in #409, but still used in several places #467

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign communication client types #409

Redesign communication client types #409

robertbartel commented Aug 7, 2023 •

edited

Loading

robertbartel commented Aug 7, 2023

robertbartel commented Aug 10, 2023

aaraney left a comment

aaraney Aug 15, 2023

robertbartel Aug 21, 2023

aaraney Aug 22, 2023

robertbartel Aug 25, 2023 •

edited

Loading

aaraney Aug 17, 2023

robertbartel Aug 21, 2023

aaraney Aug 22, 2023

robertbartel Aug 28, 2023

aaraney Aug 17, 2023

robertbartel Aug 21, 2023

aaraney Aug 22, 2023

robertbartel Aug 25, 2023

aaraney left a comment

aaraney Sep 8, 2023

robertbartel Sep 8, 2023

aaraney Sep 8, 2023

robertbartel Sep 8, 2023

aaraney Sep 8, 2023

robertbartel Sep 8, 2023

aaraney Sep 8, 2023

robertbartel Sep 8, 2023

aaraney Sep 8, 2023

robertbartel Sep 8, 2023

aaraney left a comment

Redesign communication client types #409

Redesign communication client types #409

Conversation

robertbartel commented Aug 7, 2023 • edited Loading

robertbartel commented Aug 7, 2023

robertbartel commented Aug 10, 2023

aaraney 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

robertbartel Aug 25, 2023 • 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

aaraney 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

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

aaraney left a comment

Choose a reason for hiding this comment

robertbartel commented Aug 7, 2023 •

edited

Loading

robertbartel Aug 25, 2023 •

edited

Loading